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

Refuse to modify PVCs/PVs with a VAC associated #28

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

ConnorJC3
Copy link
Contributor

@ConnorJC3 ConnorJC3 commented Apr 19, 2024

Issue #, if available:

N/A

Description of changes:

Refuses to modify volumes with a VAC associated, thus preventing multiple modifiers fighting over the same volume. We do not intend to support modifying volumes that are VAC-managed with volume-modifier-for-k8s.

(Also bumps all the dependencies which was needed to get the VolumeAttributesClassName field available in client-go)

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

torredil
torredil previously approved these changes Apr 19, 2024
@AndrewSirenko
Copy link
Contributor

Works on live cluster:

│ Name:          ebs-claim                                                                                                                                                     │
│ Namespace:     default                                                                                                                                                       │
│ StorageClass:  ebs-sc                                                                                                                                                        │
│ Status:        Bound                                                                                                                                                         │
│ Volume:        pvc-b9c51d6b-c57d-45c0-962d-c167d37f2f79                                                                                                                      │
│ Labels:        <none>                                                                                                                                                        │
│ Annotations:   ebs.csi.aws.com/iops: 4000                                                                                                                                    │
│                pv.kubernetes.io/bind-completed: yes                                                                                                                          │
│                pv.kubernetes.io/bound-by-controller: yes                                                                                                                     │
│                volume.beta.kubernetes.io/storage-provisioner: ebs.csi.aws.com                                                                                                │
│                volume.kubernetes.io/selected-node: i-09168d577ef0ac8fc                                                                                                       │
│                volume.kubernetes.io/storage-provisioner: ebs.csi.aws.com                                                                                                     │
│ Finalizers:    [kubernetes.io/pvc-protection]                                                                                                                                │
│ Capacity:      100Gi                                                                                                                                                         │
│ Access Modes:  RWO                                                                                                                                                           │
│ VolumeMode:    Filesystem                                                                                                                                                    │
│ Used By:       app                                                                                                                                                           │
│ Events:                                                                                                                                                                      │
│   Type     Reason                    Age                  From                                                                                      Message                  │
│   ----     ------                    ----                 ----                                                                                      -------                  │
│   Normal   WaitForFirstConsumer      2m9s                 persistentvolume-controller                                                               waiting for first consum │
│ er to be created before binding                                                                                                                                              │
│   Normal   ExternalProvisioning      2m9s (x2 over 2m9s)  persistentvolume-controller                                                               Waiting for a volume to  │
│ be created either by the external provisioner 'ebs.csi.aws.com' or manually by the system administrator. If volume creation is delayed, please verify that the provisioner i │
│ s running and correctly registered.                                                                                                                                          │
│   Normal   Provisioning              2m9s                 ebs.csi.aws.com_ebs-csi-controller-5c4588b657-5crpc_7b59ead5-5887-4a3a-a3fe-75536083acb9  External provisioner is  │
│ provisioning volume for claim "default/ebs-claim"                                                                                                                            │
│   Normal   ProvisioningSucceeded     2m4s                 ebs.csi.aws.com_ebs-csi-controller-5c4588b657-5crpc_7b59ead5-5887-4a3a-a3fe-75536083acb9  Successfully provisioned │
│  volume pvc-b9c51d6b-c57d-45c0-962d-c167d37f2f79                                                                                                                             │
│   Normal   VolumeModify              114s                 external-resizer ebs.csi.aws.com                                                          external resizer is modi │
│ fying volume ebs-claim with vac io2-class                                                                                                                                    │
│   Normal   VolumeModifySuccessful    106s                 external-resizer ebs.csi.aws.com                                                          external resizer modifie │
│ d volume ebs-claim with vac io2-class successfully                                                                                                                           │
│   Warning  VolumeModificationFailed  39s                  volume-modifier-for-k8s-ebs.csi.aws.com                                                   Refusing to modify pvc-b │
│ 9c51d6b-c57d-45c0-962d-c167d37f2f79 because PVC ebs-claim has a VAC associated                                                                                               │
│

Copy link
Contributor

@AndrewSirenko AndrewSirenko left a comment

Choose a reason for hiding this comment

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

Left one more error message suggestion but LGTM.

Currently testing that no issues will come from modifying via annotation, then VAC, but that test will take 5 more hours... (Out of scope for this PR, but in scope for new release of sidecar)

Thanks!

@@ -261,6 +261,14 @@ func (c *modifyController) modifyPVC(pv *v1.PersistentVolume, pvc *v1.Persistent
c.addPVCToInProgressList(pvc.Name)
defer c.removePVCFromInProgressList(pvc.Name)

if pvc.Spec.VolumeAttributesClassName != nil && *pvc.Spec.VolumeAttributesClassName != "" {
c.eventRecorder.Eventf(pvc, v1.EventTypeWarning, VolumeModificationFailed, "Refusing to modify %s because PVC %s has a VAC associated", pv.Name, pvc.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

For increased clarity, consider adding 'via PVC annotation': "Refusing to modify %s because PVC %s has a VAC associated" -> "Refusing to modify %s via PVC annotation because PVC %s has a VAC associated"

Similar change for lines 265, 266, 268, 269.

I'm expecting some poor souls to forget there are two different ways to modify a volume.

@AndrewSirenko
Copy link
Contributor

/approve

@torredil torredil merged commit cd040ad into awslabs:main Apr 22, 2024
1 check passed
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.

3 participants