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

p0upgradeK8sVersionChecks fix #233

Merged
merged 1 commit into from
Jan 8, 2025
Merged

p0upgradeK8sVersionChecks fix #233

merged 1 commit into from
Jan 8, 2025

Conversation

cpinjani
Copy link
Collaborator

@cpinjani cpinjani commented Jan 6, 2025

What does this PR do?

To include p0upgradeK8sVersionChecks for import tests

Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #198

Checklist:

@cpinjani cpinjani self-assigned this Jan 6, 2025
@cpinjani cpinjani changed the title p0upgradeK8sVersionChecks fix WIP: p0upgradeK8sVersionChecks fix Jan 6, 2025
@cpinjani
Copy link
Collaborator Author

cpinjani commented Jan 6, 2025

valaparthvi
valaparthvi previously approved these changes Jan 6, 2025
@valaparthvi valaparthvi self-requested a review January 6, 2025 10:59
@valaparthvi valaparthvi dismissed their stale review January 6, 2025 10:59

Dismissing review due to P1 import failures.

@cpinjani cpinjani force-pushed the eks-p0-import branch 2 times, most recently from b675fc0 to 9f89427 Compare January 7, 2025 05:02
@cpinjani cpinjani added enhancement New feature or request area/testing labels Jan 7, 2025
@cpinjani cpinjani force-pushed the eks-p0-import branch 8 times, most recently from 53e2f89 to 9a8a935 Compare January 7, 2025 15:14
@cpinjani cpinjani changed the title WIP: p0upgradeK8sVersionChecks fix p0upgradeK8sVersionChecks fix Jan 7, 2025
cluster, err = client.Management.Cluster.ByID(cluster.ID)
Expect(err).To(BeNil())
return *cluster.EKSStatus.UpstreamSpec.KubernetesVersion
}, tools.SetTimeout(15*time.Minute), 30*time.Second).Should(Equal(upgradeToVersion))
return *cluster.EKSStatus.UpstreamSpec.KubernetesVersion == upgradeToVersion && *cluster.EKSConfig.KubernetesVersion == upgradeToVersion
Copy link
Collaborator

@valaparthvi valaparthvi Jan 8, 2025

Choose a reason for hiding this comment

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

I don't think we need to check EKSConfig again, since we are already checking in on L80.

Copy link
Collaborator Author

@cpinjani cpinjani Jan 8, 2025

Choose a reason for hiding this comment

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

True, however eks-operator/issues/668 (fixed already) occurs sometimes due to race condition.
This check ensures condition is met to start Nodegroup upgrade.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind adding a comment about it if you haven't already? It's possible that I or someone else might forget later and remove it.

Copy link
Collaborator

@valaparthvi valaparthvi left a comment

Choose a reason for hiding this comment

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

All looks good and the tests are also passing, apart from the one comment.

Signed-off-by: Chandan Pinjani <chandan.pinjani@suse.com>
@cpinjani cpinjani merged commit 6a72c34 into main Jan 8, 2025
2 checks passed
@cpinjani cpinjani deleted the eks-p0-import branch January 8, 2025 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Refactor p0upgradeK8sVersionChecks function for imported EKS cluster
2 participants