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

432 Support upgrade for existing RGs with Init Container #76

Merged
merged 16 commits into from
Feb 14, 2023

Conversation

santhoshatdell
Copy link
Contributor

@santhoshatdell santhoshatdell commented Feb 7, 2023

Description

Replication APIs are updated from v1alpha1 to v1 (#75).
Changes in this PR are related to Init Container which migrates the existing RG's apiVersion & CRD's to v1 during upgrade.
For fresh installations, init container will not do anything as no existing RGs will be found.

To follow-up:

  • Base image may need to be changed. Currently bitnami/kubectl is used.
  • Need new Docker Hub repository for new image (dellemc/dell-replication-init).

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#432

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate (81.8%)
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have maintained backward compatibility (via Init Container which will migrate RG & CRD versions during upgrade. With that, existing RGs continue to work).

How Has This Been Tested?

  • Installed CSM 1.5 replication module with isilon driver, and provisioned few PV/RGs.
  • Upgraded isilon & replicator sidecar images to latest on both k8s clusters.
    cd dell-csi-helm-installer && ./csi-install.sh --namespace isilon --values /home/santhosh/my-samples/csm-replication/isilon-values.yaml --upgrade
  • Upgraded replication-controller images.
    ./repctl create -f ../deploy/controller.yaml
  • Verified RGs and CRD for v1 version with init container logs and kubectl on both clusters.
  • Deleted target PowerScale SyncIQ policies.
  • Restarted pod to confirm no migration was run second time.
  • Provisioned new RG and ran replication actions on existing & new RGs.

Init container logs migrating alpha to v1

Init container logs skipping migration

Base automatically changed from 432-alpha-api to main February 8, 2023 23:23
harshaatdell
harshaatdell previously approved these changes Feb 10, 2023
Copy link

@harshaatdell harshaatdell left a comment

Choose a reason for hiding this comment

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

Thank you

config/manager/manager.yaml Outdated Show resolved Hide resolved
deploy/controller.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,6 @@
# Init container
FROM bitnami/kubectl:1.25.6 AS init
Copy link
Contributor

Choose a reason for hiding this comment

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

The ubi-minimal image can be used as a base image, since that will be required for operator certification. In this case, you would need to install the kubectl binary during image creation. See here for reference to the current ubi-minimal image that is being used: https://github.com/dell/csi-powerflex/blob/main/overrides.mk#L6

Copy link
Contributor Author

@santhoshatdell santhoshatdell Feb 10, 2023

Choose a reason for hiding this comment

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

We got to know about the base image requirement recently. @donatwork is currently working on common dell/kubectl image which we can switch to as needed.
If the init container is going to be around for few releases, I am not sure if it has to be certified. Moreover, rest of the replication images are built with scratch. When we must switch all images to use ubi-minimal as base, I can work on converting all at that time.

Copy link

@rbo54 rbo54 left a comment

Choose a reason for hiding this comment

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

I agree with Trevor's comments on semantic versioning and ubi-minimal. I didn't find anything additional.

@santhoshatdell santhoshatdell merged commit 6449082 into main Feb 14, 2023
@santhoshatdell santhoshatdell deleted the upgrade-initImage branch February 14, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants