Fix volume creation for PVC

[dsimansk]

Description

When initializing volumes for flag usage like in E2E --mount /mydir5=pvc:test-pvc. We shouldn't probably enforce readOnly: true on the created volume PVC. We may end up with service stuck in initialization phase, because of underlying storage config, e.g. some volumes are being formatted first.

Error from volume init:

  Warning  FailedMount             1s (x4 over 7s)  kubelet                  MountVolume.MountDevice failed for volume "pvc-1577f113-81c1-44f9-861f-5def40d13e8b" : cannot mount unformatted disk /dev/nvme2n1 as we are manipulating it in read-only mode

Former behavior:

        volumes:
        - name: mydir5-aa14be68
          persistentVolumeClaim:
            claimName: test-pvc
            readOnly: true

/cc @vyasgun @rhuss @skonto

Changes

• Fix volume creation for PVC

Reference

Fixes #

Release Note

Fix mounted PVC not to be created as read-only

/kind bug

[knative-prow]

@dsimansk: 0 warnings.

In response to this:

Description

When initializing volumes for flag usage like in E2E --mount /mydir5=pvc:test-pvc. We shouldn't probably enforce readOnly: true on the created volume PVC. We may end up with service stuck in initialization phase, because of underlying storage config, e.g. some volumes are being formatted first.

Error from volume init:

 Warning  FailedMount             1s (x4 over 7s)  kubelet                  MountVolume.MountDevice failed for volume "pvc-1577f113-81c1-44f9-861f-5def40d13e8b" : cannot mount unformatted disk /dev/nvme2n1 as we are manipulating it in read-only mode

Former behavior:

       volumes:
       - name: mydir5-aa14be68
         persistentVolumeClaim:
           claimName: test-pvc
           readOnly: true

/cc @vyasgun @rhuss @skonto

Changes

• Fix volume creation for PVC

Reference

Fixes #

Release Note

Fix mounted PVC not to be create as read-only

/kind bug

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.

[codecov]

Codecov Report

Merging #1709 (e2df5d5) into main (ed5fed5) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1709   +/-   ##
=======================================
  Coverage   79.91%   79.91%           
=======================================
  Files         174      174           
  Lines       13515    13515           
=======================================
  Hits        10800    10800           
  Misses       1978     1978           
  Partials      737      737           

Impacted Files	Coverage Δ
pkg/kn/flags/podspec_helper.go	85.21% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed5fed5...e2df5d5. Read the comment docs.

[rhuss]

Looks good to me to change the default to not provide readOnly: true (what was the motivation in the first place btw ? as I think a readOnly PV doesn't make much sense for the usual use case anyway)

But would it make sense to allow options to be specified for a mount ? (like with appending a ,readOnly=true to the spec ?) If so, we should create a followup issue for it, I'm good with merging this PR as is (but up to you)

/approve
/lgtm
/hold

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, rhuss

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:

• OWNERS [dsimansk,rhuss]

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

[vyasgun]

@rhuss I think I added the readOnly: true flag as default because it was the default for other types of volume mounts. There was also some discussion about adding a flag to configure the readOnly attribute.

[dsimansk]

Looks good to me to change the default to not provide readOnly: true (what was the motivation in the first place btw ? as I think a readOnly PV doesn't make much sense for the usual use case anyway)

But would it make sense to allow options to be specified for a mount ? (like with appending a ,readOnly=true to the spec ?) If so, we should create a followup issue for it, I'm good with merging this PR as is (but up to you)

/approve /lgtm /hold

IMO it probably slipped through review. We had a few places that specified volumes as readOnly because of Secrets and ConfigMaps being a primary target previously.

Indeed we should look at adding readOnly to a volume config flag to cover that case. However, most of the time I'd expect PVC needs to writable.

[rhuss]

@rhuss I think I added the readOnly: true flag as default because it was the default for other types of volume mounts. There was also some discussion about adding a flag to configure the readOnly attribute.

Do you know whether we have opened an issue for that ? If not, we should.

[dsimansk]

Here: #1712

/unhold

[dsimansk]

/cherry-pick release-1.6

[knative-prow-robot]

@dsimansk: #1709 failed to apply on top of branch "release-1.6":

Applying: Fix volume creation for PVC
Using index info to reconstruct a base tree...
M	test/e2e/service_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/service_test.go
CONFLICT (content): Merge conflict in test/e2e/service_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix volume creation for PVC
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.6

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.

[dsimansk]

/cherry-pick release-1.6

[knative-prow-robot]

@dsimansk: new pull request created: #1723

In response to this:

/cherry-pick release-1.6

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.