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

Update kube-vip for InPlace upgrades. #7595

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

rahulbabu95
Copy link
Member

@rahulbabu95 rahulbabu95 commented Feb 15, 2024

Issue #, if available:
Kube-vip is deployed as static pod which means during any eksd/k8s version upgrade InPlace, we need to also update the kube-vip manifest with the new image for InPlace upgrades. This change parses the udpated Kube-vip manifest from KubeadmControlPlaneSpec.KubeadmConfigSpec and uses a config map mounted to the upgrader pod to facilitate kube-vip update for InPlace. The upgrader script then uses the volume mount to update the kube-vip manifest onto the host.

Description of changes:

Testing (if applicable):
Tested the changes locally.

Documentation added/planned (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eks-distro-bot eks-distro-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 15, 2024
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (4583834) 73.48% compared to head (c6c5b9c) 73.59%.
Report is 81 commits behind head on main.

❗ Current head c6c5b9c differs from pull request most recent head 1996a5f. Consider uploading reports for the commit 1996a5f to get more accurate results

Files Patch % Lines
controllers/controlplaneupgrade_controller.go 78.57% 9 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7595      +/-   ##
==========================================
+ Coverage   73.48%   73.59%   +0.10%     
==========================================
  Files         579      580       +1     
  Lines       36357    36722     +365     
==========================================
+ Hits        26718    27024     +306     
- Misses       7875     7919      +44     
- Partials     1764     1779      +15     

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

ObjectMeta: metav1.ObjectMeta{
Name: constants.KubeVipConfigMapName,
Namespace: constants.EksaSystemNamespace,
OwnerReferences: []metav1.OwnerReference{{
Copy link
Member

Choose a reason for hiding this comment

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

This is good but this doesn't ensure it gets cleaned up right away right? Meaning, if a new CPUpgrade needs to get created with the same name due to some subsequent upgrade, we will never get the new CM for this if it never gets deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. I will clean it up once all the nodes are upgraded for a given cpu. This way we ensure we always clean it after a successful upgrade.

Copy link
Member

Choose a reason for hiding this comment

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

I think when you have BlockOwnerDeletion set on the child object, it blocks the parent object deletion. @rahulbabu95 you can test it out just to be sure

@drewvanstone drewvanstone added this to the v0.19.0 milestone Feb 15, 2024
@rahulbabu95 rahulbabu95 force-pushed the inplace/update-kubevip-manifest branch from 439eecb to c6c5b9c Compare February 15, 2024 19:00
@rahulbabu95
Copy link
Member Author

/retest

cm := &corev1.ConfigMap{}
if err := client.Get(ctx, GetNamespacedNameType(constants.KubeVipConfigMapName, constants.EksaSystemNamespace), cm); err != nil {
if apierrors.IsNotFound(err) {
log.Info("config map %s not found, skipping deletion", "ConfigMap", constants.KubeVipConfigMapName, "Namespace", constants.EksaSystemNamespace)
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
log.Info("config map %s not found, skipping deletion", "ConfigMap", constants.KubeVipConfigMapName, "Namespace", constants.EksaSystemNamespace)
log.Info("config map %s not found, skipping deletion", "ConfigMap", constants.KubeVipConfigMapName, "Namespace", constants.EksaSystemNamespace)

kubeVipConfig = file.Content
break
}
}
Copy link
Member

Choose a reason for hiding this comment

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

can you add a check here to see kubeVipConfig is not empty after the for loop? incase the kube-vip is not found in files, just error out

@@ -48,6 +48,9 @@ const (
EksaPackagesName = "eksa-packages"
// UpgraderConfigMapName is the name of config map that stores the upgrader images.
UpgraderConfigMapName = "in-place-upgrade"
// KubeVipConfigMapName is the name of config map that stores the kube-vip config.
KubeVipConfigMapName = "kube-vip-in-place-upgrade"
KubeVipManifestName = "kube-vip.yaml"
Copy link
Member

Choose a reason for hiding this comment

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

nit: add comment for KubeVipManifestName

@rahulbabu95
Copy link
Member Author

/retest

1 similar comment
@abhinavmpandey08
Copy link
Member

/retest

@rahulbabu95
Copy link
Member Author

/approve

@eks-distro-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rahulbabu95

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:

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

@rahulbabu95
Copy link
Member Author

/override eks-anywhere-e2e-presubmit

@eks-distro-bot
Copy link
Collaborator

@rahulbabu95: Overrode contexts on behalf of rahulbabu95: eks-anywhere-e2e-presubmit

In response to this:

/override eks-anywhere-e2e-presubmit

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/test-infra repository.

@eks-distro-bot eks-distro-bot merged commit 09d59ce into aws:main Feb 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants