Task/NVPE-32: Cleanup NIM integration tech preview resources #1369

TomerFi · 2024-11-13T00:56:38Z

Description

Version 2.16 will include the GA version of NVIDIA NIM integration. Cleaning up the TP version's resources introduced in 2.15 is required.

https://issues.redhat.com/browse/NVPE-32

How Has This Been Tested?

go fmt
make lint
make test
go mod tidy

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>

openshift-ci · 2024-11-13T00:56:50Z

Hi @TomerFi. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

pkg/upgrade/upgrade.go

mpaulgreen · 2024-11-13T10:58:41Z

pkg/upgrade/upgrade.go


+	// cleanup nvidia nim integration remove tech preview
+	multiErr = multierror.Append(multiErr, cleanupNimIntegrationTechPreview(ctx, cli, oldReleaseVersion))


@andrewballantyne dont we need to cleanup api key secret also? AFAIU UI will create the secret when user selects the enable button. So, if the secret is present then it will result in an error.

Yeah, that's a good catch. If we don't delete the secret and we don't have a NIM Account CR here... it will likely block the creation flow. @etirelli should we delete the customer's "key file" or look at some what to keep/update it on install in the new Controller way?

I would say update/reuse would be best as it is unlikely the API key itself changed and forcing the customer to update might create confusion.

discussed in slack. We will delete the secret if it exists and let the user recreate it.

Resolved in 0483894.

mpaulgreen · 2024-11-13T11:12:12Z

cc: @lburgazzoli @israel-hdez

Co-authored-by: Wen Zhou <wenzhou@redhat.com>

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>

pkg/upgrade/upgrade.go

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>

codecov · 2024-11-13T16:24:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (incubation@e25487a). Learn more about missing BASE report.

Additional details and impacted files

@@              Coverage Diff              @@
##             incubation    #1369   +/-   ##
=============================================
  Coverage              ?   18.96%           
=============================================
  Files                 ?       30           
  Lines                 ?     3390           
  Branches              ?        0           
=============================================
  Hits                  ?      643           
  Misses                ?     2678           
  Partials              ?       69

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zdtsw · 2024-11-13T16:27:03Z

/ok-to-test

etirelli

lgtm

openshift-ci · 2024-11-13T16:31:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: etirelli

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [etirelli]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zdtsw · 2024-11-13T17:35:41Z

put on-hold,
i will need to test a bit more tomorrow morning + give other reviewers time to put comments if any.

TomerFi · 2024-11-13T22:30:53Z

/retest

zdtsw · 2024-11-14T09:46:40Z

test:

install 2.15.0 ODH do clean install, creation DSCI,

create dummy secret "nvidia-nim-access" in opendatahub NS
create dummy configmap "nvidia-nim-validation-result" in opendatahub NS
create dummy cronjob "nvidia-nim-periodic-validator" in opendatahub NS
uninstall ODH, keep DSCI in cluster

build 2.16.1114 with PR, install
check log:

{"level":"info","ts":"2024-11-14T09:40:55Z","msg":"removed NIM validator CronJob successfully"}
{"level":"info","ts":"2024-11-14T09:40:55Z","msg":"removed NIM data ConfigMap successfully"}
{"level":"info","ts":"2024-11-14T09:40:55Z","msg":"removed NIM API key Secret successfully"}
check above resourcs gone

zdtsw · 2024-11-14T09:49:05Z

in short, test result looks good

openshift-ci · 2024-11-14T09:51:09Z

New changes are detected. LGTM label has been removed.

…ahub-io#1369) * chore: cleanup nim integration tech preview resources Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> * chore: apply suggestions from code review Co-authored-by: Wen Zhou <wenzhou@redhat.com> * chore: addressed pr reviews, better logging Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> * chore: set nim cleanup for minors 14 and 15 Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> * chore: nim tp cleanup should remove api key secret Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> * chore: addresed pr reviews, rewrite cleanup func Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> --------- Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> Co-authored-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 84d22f3)

* Task/NVPE-32: Cleanup NIM integration tech preview resources (#1369) * chore: cleanup nim integration tech preview resources Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> * chore: apply suggestions from code review Co-authored-by: Wen Zhou <wenzhou@redhat.com> * chore: addressed pr reviews, better logging Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> * chore: set nim cleanup for minors 14 and 15 Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> * chore: nim tp cleanup should remove api key secret Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> * chore: addresed pr reviews, rewrite cleanup func Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> --------- Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com> Co-authored-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 84d22f3) * update: use old logger Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> Co-authored-by: Tomer Figenblat <tomer.figenblat@gmail.com>

chore: cleanup nim integration tech preview resources

e322b31

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress label Nov 13, 2024

openshift-ci bot added the needs-ok-to-test label Nov 13, 2024

zdtsw reviewed Nov 13, 2024

View reviewed changes

mpaulgreen reviewed Nov 13, 2024

View reviewed changes

TomerFi and others added 4 commits November 13, 2024 08:30

chore: apply suggestions from code review

85077d0

Co-authored-by: Wen Zhou <wenzhou@redhat.com>

chore: addressed pr reviews, better logging

837fee0

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>

chore: set nim cleanup for minors 14 and 15

e808e82

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>

chore: nim tp cleanup should remove api key secret

0483894

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>

TomerFi marked this pull request as ready for review November 13, 2024 14:33

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 13, 2024

openshift-ci bot requested review from adelton and MarianMacik November 13, 2024 14:33

zdtsw reviewed Nov 13, 2024

View reviewed changes

pkg/upgrade/upgrade.go Outdated Show resolved Hide resolved

pkg/upgrade/upgrade.go Outdated Show resolved Hide resolved

pkg/upgrade/upgrade.go Outdated Show resolved Hide resolved

chore: addresed pr reviews, rewrite cleanup func

610ea2e

Signed-off-by: Tomer Figenblat <tfigenbl@redhat.com>

TomerFi force-pushed the cleanup-nim-tp branch from 4d617c5 to 610ea2e Compare November 13, 2024 16:18

zdtsw requested review from lburgazzoli, etirelli, zdtsw, andrewballantyne and mpaulgreen and removed request for adelton and MarianMacik November 13, 2024 16:22

zdtsw removed the needs-ok-to-test label Nov 13, 2024

openshift-ci bot added the ok-to-test Ok to Test label Nov 13, 2024

etirelli approved these changes Nov 13, 2024

View reviewed changes

openshift-ci bot assigned etirelli Nov 13, 2024

openshift-ci bot added the lgtm label Nov 13, 2024

openshift-ci bot added the approved label Nov 13, 2024

TomerFi changed the title ~~chore: cleanup nim integration tech preview resources~~ Task/NVPE-32: Cleanup NIM integration tech preview resources Nov 13, 2024

zdtsw added the do-not-merge/hold label Nov 13, 2024

zdtsw removed the do-not-merge/hold label Nov 14, 2024

Merge branch 'incubation' into cleanup-nim-tp

78dd970

openshift-ci bot removed the lgtm label Nov 14, 2024

zdtsw merged commit 84d22f3 into opendatahub-io:incubation Nov 14, 2024
1 of 6 checks passed

zdtsw mentioned this pull request Nov 14, 2024

[sync]: Cleanup NIM integration tech preview resources #1372

Merged

5 tasks

TomerFi deleted the cleanup-nim-tp branch November 14, 2024 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task/NVPE-32: Cleanup NIM integration tech preview resources #1369

Task/NVPE-32: Cleanup NIM integration tech preview resources #1369

TomerFi commented Nov 13, 2024

openshift-ci bot commented Nov 13, 2024

mpaulgreen Nov 13, 2024

andrewballantyne Nov 13, 2024

etirelli Nov 13, 2024

etirelli Nov 13, 2024

TomerFi Nov 13, 2024

mpaulgreen commented Nov 13, 2024

codecov bot commented Nov 13, 2024

zdtsw commented Nov 13, 2024

etirelli left a comment

openshift-ci bot commented Nov 13, 2024

zdtsw commented Nov 13, 2024

TomerFi commented Nov 13, 2024

zdtsw commented Nov 14, 2024

zdtsw commented Nov 14, 2024

openshift-ci bot commented Nov 14, 2024


		// cleanup nvidia nim integration remove tech preview
		multiErr = multierror.Append(multiErr, cleanupNimIntegrationTechPreview(ctx, cli, oldReleaseVersion))

Task/NVPE-32: Cleanup NIM integration tech preview resources #1369

Task/NVPE-32: Cleanup NIM integration tech preview resources #1369

Conversation

TomerFi commented Nov 13, 2024

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

openshift-ci bot commented Nov 13, 2024

mpaulgreen Nov 13, 2024

Choose a reason for hiding this comment

andrewballantyne Nov 13, 2024

Choose a reason for hiding this comment

etirelli Nov 13, 2024

Choose a reason for hiding this comment

etirelli Nov 13, 2024

Choose a reason for hiding this comment

TomerFi Nov 13, 2024

Choose a reason for hiding this comment

mpaulgreen commented Nov 13, 2024

codecov bot commented Nov 13, 2024

Codecov Report

zdtsw commented Nov 13, 2024

etirelli left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 13, 2024

zdtsw commented Nov 13, 2024

TomerFi commented Nov 13, 2024

zdtsw commented Nov 14, 2024

zdtsw commented Nov 14, 2024

openshift-ci bot commented Nov 14, 2024