Skip to content

test: cover storage access mode defaulting - #452

Open
mehrdadbn9 wants to merge 1 commit into
etcd-io:mainfrom
mehrdadbn9:test/storage-access-modes-107
Open

test: cover storage access mode defaulting#452
mehrdadbn9 wants to merge 1 commit into
etcd-io:mainfrom
mehrdadbn9:test/storage-access-modes-107

Conversation

@mehrdadbn9

Copy link
Copy Markdown

storageSpec.accessModes is optional, and leaving it unset used to produce a volume claim template with accessModes: [""], which the API server rejects, so every pod stayed pending. That is what #107 reported.

It was already fixed in 3905389 ("Defaults to ReadWriteOnce when it's empty or not provided"), which is why the recent attempts to reproduce it came back clean. Nothing covers that defaulting though, so it can be dropped again without a test going red - the e2e cases always set accessModes: ReadWriteOnce explicitly, so they never touch the empty path.

This adds a table-driven test over createOrPatchStatefulSet for the access mode branches:

Verification

Reverting 3905389 locally (putting ec.Spec.StorageSpec.AccessModes back in the claim template) turns exactly the first case red and leaves the other four green:

--- FAIL: TestCreateOrPatchStatefulSetStorageAccessModes/defaults_to_ReadWriteOnce_when_accessModes_is_not_provided
    Error: Not equal:
      expected: []v1.PersistentVolumeAccessMode{"ReadWriteOnce"}
      actual  : []v1.PersistentVolumeAccessMode{""}

which is the same accessModes: [''] from the original report. With the fix in place the whole package is green, and make lint reports 0 issues.

Test only, no production code touched.

Refs #107

The PVC access mode used to be copied straight out of the EtcdCluster
spec, so leaving storageSpec.accessModes unset produced a volume claim
template with accessModes: [""]. The API server rejects that, and every
pod stays pending with:

  PersistentVolumeClaim "etcd-data-example-0" is invalid:
  spec.accessModes: Unsupported value: ""

That was fixed in 3905389 by hardcoding ReadWriteOnce for the empty and
ReadWriteOnce cases, but nothing covers it, so the defaulting can be
dropped again without a test failing. The e2e cases always set the
access mode explicitly, so they never exercise the empty path.

Add a table-driven test over createOrPatchStatefulSet covering the
access mode branches: unset defaults to ReadWriteOnce, an explicit
ReadWriteOnce is kept, ReadWriteMany mounts the named PVC instead of
generating a claim template, ReadWriteMany without a PVC name is
rejected, and an unsupported mode is rejected. Reverting 3905389
locally turns the first case red with actual: [""], so it guards the
original report.

Refs etcd-io#107

Signed-off-by: mehrdadbn9 <mehrdadbiukian@gmail.com>
@kubernetes-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mehrdadbn9
Once this PR has been reviewed and has the lgtm label, please assign ivanvc for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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

@kubernetes-prow

Copy link
Copy Markdown

Hi @mehrdadbn9. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant