Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NAS-120628 / 23.10 / Add IPFS app to community train #1023

Merged
merged 5 commits into from
Mar 20, 2023
Merged

NAS-120628 / 23.10 / Add IPFS app to community train #1023

merged 5 commits into from
Mar 20, 2023

Conversation

stavros-k
Copy link
Contributor

@stavros-k stavros-k commented Mar 6, 2023 •
edited
Loading

  • Run as non-root (expect permissions container)
  • Permissions containers has its Resources is limited to 512M + 1vcpu
  • Configurable ports (applies when hostnet is enabled too)
  • Allow for user defined env vars
  • Apply port/headers configuration on each startup (Resources is limited to 512M + 1vcpu)
    • Maybe make headers optional?
  • Allow custom config commands to be appended on the init script? Can be quite challenging for json payload. as users will be expected to correctly escape double quotes.

@stavros-k stavros-k added the jira label Mar 6, 2023
@stavros-k stavros-k self-assigned this Mar 6, 2023
@bugclerk bugclerk changed the title Add IPFS app to community train NAS-120628 / 23.10 / Add IPFS app to community train Mar 6, 2023
@bugclerk
Copy link
Contributor

bugclerk commented Mar 6, 2023

Jira URL: https://ixsystems.atlassian.net/browse/NAS-120628

@stavros-k stavros-k changed the base branch from master to common-fixes-improvements March 6, 2023 16:31
@stavros-k stavros-k force-pushed the ipfs-community branch 4 times, most recently from ae299d9 to c3de3eb Compare March 8, 2023 13:28
stavros-k
stavros-k commented Mar 8, 2023
View reviewed changes
catalog.json Outdated Show resolved Hide resolved
@stavros-k stavros-k force-pushed the ipfs-community branch 2 times, most recently from 1db19d3 to c92bf3e Compare March 8, 2023 15:39
@stavros-k stavros-k marked this pull request as ready for review March 8, 2023 15:47
@stavros-k stavros-k force-pushed the common-fixes-improvements branch 6 times, most recently from 2744c08 to 024b5f1 Compare March 9, 2023 07:40
Qubad786
Qubad786 reviewed Mar 9, 2023
View reviewed changes
library/community/ipfs/values.yaml Outdated Show resolved Hide resolved
library/community/ipfs/questions.yaml Outdated
required: true

- variable: resources
label: ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing label

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's on purpose, As it's the same as the group name. And it just shows the same text twice

library/community/ipfs/questions.yaml Outdated Show resolved Hide resolved
@stavros-k stavros-k force-pushed the ipfs-community branch 3 times, most recently from be453ee to 9366e90 Compare March 11, 2023 12:53
@stavros-k stavros-k force-pushed the ipfs-community branch 3 times, most recently from 3c06e5b to 4b5ba07 Compare March 13, 2023 13:33
@stavros-k stavros-k changed the base branch from common-fixes-improvements to master March 16, 2023 16:06
Qubad786
Qubad786 reviewed Mar 16, 2023
View reviewed changes
Copy link
Contributor

@Qubad786 Qubad786 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are missing upgrade strategies files so these apps won't get updates..
Also are we going to chown the storage paths?

@stavros-k
Copy link
Contributor Author

We are missing upgrade strategies files so these apps won't get updates.. Also are we going to chown the storage paths?

I'll need help with the update strategies, at least until I get how it works.

From my testing chown is needed for both host path and ixVolumes,
because the app does not runs as root.

@Qubad786
Copy link
Contributor

@stavros-k for apps which are already present in charts train like ipfs - we can copy the strategies directly and for apps which don't already have one,please create tickets for those..

Where are we doing the chown?

@stavros-k
Copy link
Contributor Author

@stavros-k for apps which are already present in charts train like ipfs - we can copy the strategies directly and for apps which don't already have one,please create tickets for those..

Where are we doing the chown?

We chown data and staging

@Qubad786
Copy link
Contributor

And this happens only once on install?

@stavros-k
Copy link
Contributor Author

And this happens only once on install?

Currently is at every startup, that was because I had to fix permissions before the init-config container runs. But permissions container was running after.

But with a little change I did in common before we merge it, I can now run it once at installation.

@Qubad786
Copy link
Contributor

I think it should be once on installation as chown can be expensive..

@stavros-k
Copy link
Contributor Author

I think it should be once on installation as chown can be expensive..

Agreed.

Qubad786
Qubad786 reviewed Mar 20, 2023
View reviewed changes
library/ix-dev/community/ipfs/templates/_ipfs.tpl
{{- include "ix.v1.common.app.permissions" (dict "containerName" "01-permissions"
"UID" .Values.ipfsRunAs.user
"GID" .Values.ipfsRunAs.group
"type" "install") | nindent 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think having another workload which only targets the install time workload makes more sense?
It feels weird that we have a workload defined which will be actually running ipfs and then we have under init container some metadata which says that this is only applicable on installatino of chart?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the 01-permissions container is marked as type: install. Which common puts it between an if block of .Release.IsInstall.
Meaning it will only render at installation, upgrades will not render this at all.
ipfs container does not run as root, so it won't be able to do any chown.

The 02-init-config is type: init, meaning it will get rendered (and run on every start of the app).
Which in this case is needed as it configures the ports.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see..i was seeing this differently as being a pre-install job or something but obviously that is not the case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop, it's just containers in the same Deployment.

Opted to not go for jobs in this case, as it's quick actions. And spinning a whole extra pod would require more configuration

@stavros-k stavros-k merged commit 010418b into master Mar 20, 2023
@stavros-k stavros-k deleted the ipfs-community branch March 20, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qubad786 Qubad786 Qubad786 approved these changes

Assignees

@stavros-k stavros-k

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stavros-k @bugclerk @Qubad786