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.

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

feat: add sa token component to kfp-persistence #349

Merged
merged 6 commits into from
Oct 19, 2023

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Oct 11, 2023

The kfp-persistence charms needs a ServiceAccount token to be inside the workload container to be able to start the service. The sa token component will generate the required token and save it in a file where it can be accessed to later be pushed from the charm to the workload container.

Testing

  1. Build and deploy the charm in this PR
  2. Using kubectl exec check the files inside the workload container. Make sure you check the persistenceagent container and not the charm.
  3. There must be a non empty token file in /var/run/secrets/kubeflow/tokens/<token name>

Fixes #343

The kfp-persistence charms needs a ServiceAccount token to be
inside the workload container to be able to start the service.
The sa token component will generate the required token and save it
in a file where it can be accessed to later be pushed from the charm
to the workload container.

Fixes #343
@DnPlas DnPlas requested a review from a team as a code owner October 11, 2023 10:18
@DnPlas DnPlas changed the base branch from main to 1.8-updates-dev-branch October 11, 2023 10:24
Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

So left one more comment which is more of base charm question as to if this should be the way to implement the component. The logic in the rest of the code looks good.

I will go ahead and deploy charms from the PR in order to verify the functionality. My understanding is that I should:

  • look for the file that the component is expected to create
  • check if kfp-persistence service has started successfully

Is there something else I should look into?

@DnPlas
Copy link
Contributor Author

DnPlas commented Oct 18, 2023

So left one more comment which is more of base charm question as to if this should be the way to implement the component. The logic in the rest of the code looks good.

I will go ahead and deploy charms from the PR in order to verify the functionality. My understanding is that I should:

* look for the file that the component is expected to create

* check if kfp-persistence service has started successfully

Is there something else I should look into?

Hey @orfeas-k, I added a "testing" section in the description of the PR. Regarding checking that the service starts, in this case, we'll see the service starting just fine because in pipelines 2.0.0-alpha.7 it does not need this file. We will observe this dependency when we finalise the work in #331. For now just ensuring the file is there is enough.

Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

Great work and collaboration @DnPlas, approving!

@DnPlas DnPlas merged commit a244ffe into 1.8-updates-dev-branch Oct 19, 2023
40 checks passed
@DnPlas DnPlas deleted the KF-4495-fix-343 branch October 19, 2023 09:11
DnPlas added a commit that referenced this pull request Nov 20, 2023
* feat: add sa token component to kfp-persistence

The kfp-persistence charms needs a ServiceAccount token to be
inside the workload container to be able to start the service.
The sa token component will generate the required token and save it
in a file where it can be accessed to later be pushed from the charm
to the workload container.

Fixes #343

* tests: use mysql 8.0/stable for integration tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kfp-persistence stuck in waiting status
2 participants