Update for latest CSI driver image post issues/167 #17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since the [fix](kubernetes-sigs/aws-efs-csi-driver#185) for [issue #167](kubernetes-sigs/aws-efs-csi-driver#167) merged, the AWS EFS CSI driver overwrote the [`latest` image tag](sha256-962619a5deb34e1c4257f2120dd941ab026fc96adde003e27f70b65023af5a07?context=explore) to include it. For starters, this means we can update this operator to use the [new method of specifying access points](https://github.com/kubernetes-sigs/aws-efs-csi-driver/tree/0ae998c5a95fe6dbee7f43c182997e64872695e6/examples/kubernetes/access_points#edit-persistent-volume-spec) via a colon-delimited `volumeHandle` as opposed to in `mountOptions`. However, the same update to `latest` also brought in a [commit](kubernetes-sigs/aws-efs-csi-driver@b3baff8) that requires an additional mount in the `efs-plugin` container of the DaemonSet. So we need to update our YAML for that resource at the same time, or everything is broken (this might be upstream [issue #192](kubernetes-sigs/aws-efs-csi-driver#192). This update to the DaemonSet YAML also syncs with [upstream](https://github.com/kubernetes-sigs/aws-efs-csi-driver/blob/0ae998c5a95fe6dbee7f43c182997e64872695e6/deploy/kubernetes/base/node.yaml) by bumping the image versions for the other two containers (csi-node-driver-registrar: v1.1.0 => v1.3.0; and livenessprobe: v1.1.0 => livenessprobe:v2.0.0).
openshift-ci-robot
added
the
approved
Codecov Report
@@ Coverage Diff @@
## master #17 +/- ##
==========================================
- Coverage 74.11% 74.06% -0.05%
==========================================
Files 18 18
Lines 618 617 -1
==========================================
- Hits 458 457 -1
Misses 153 153
Partials 7 7
|
2uasimojo
commented
Jun 24, 2020
Make these methods exported from the util package, next to the Ensurable interface, since a) a subsequent commit is going to use the former from the sharedvolume side, and b) they make sense there anyway, since there's nothing statics-specific about them.
PVs are immutable once created. This *should* mean we can never hit a code path where we would *need* to `Update` a PV (via `Ensure`). And if we did, it would bounce anyway because... PVs are immutable once created. There's a weird edge case, though: if the shape of a PV created by this operator changes from one version to the next, the latter will come up and think the PV needs to be changed to the new shape, and will try to `Update`. That would bounce and put the operator into a hot loop. So this commit simply disables `Update`s of PVs by pretending they never change. We'll still restore one if deleted (which probably also won't work, for reasons).
|
I live tested this with a couple of SV/PV/PVC groups from the previous version, and they are left alone as expected. No hot looping in the operator. Deleting the old SVs deletes their respective PV/PVC pairs without complaint. This is ready to go. |
Per [review](https://github.com/openshift/aws-efs-operator/pull/17/files#r445217067), use `fmt.Sprintf` rather than `+` to construct strings.
Some comments referred to "0.0.2" as the version before we changed the shape of PVs to use the colon-delimited VolumeHandle. But 0.0.2 refers to the [community-operators](https://github.com/operator-framework/community-operators/tree/master/community-operators/aws-efs-operator/0.0.2) (OperatorHub) version, which is just one of many possible version numbers that could refer to the same thing. Clarify these by instead referring to the commit where the change happened. Let the reader figure out what version that corresponds to in whatever context they care about.
2uasimojo
commented
Jun 25, 2020
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo, jharrington22 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:
Approvers can indicate their approval by writing |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
Grr, why isn't PR check completing? |
|
/retest |
1 similar comment
|
/retest |
2uasimojo
added a commit
to 2uasimojo/aws-efs-operator
that referenced
this pull request
Jun 30, 2020
This was missed in [openshift#17](openshift#17)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since the fix for issue #167 merged upstream, the AWS EFS CSI driver team overwrote the
latestimage tag to include it.For starters, this means we can update this operator to use the new method of specifying access points via a colon-delimited
volumeHandleas opposed to inmountOptions.However, the same update to
latestalso brought in a commit that requires an additional mount in theefs-plugincontainer of the DaemonSet. So we need to update our YAML for that resource at the same time, or everything is broken (this might be upstream issue #192). This update to the DaemonSet YAML also syncs with upstream by bumping the image versions for the other two containers (csi-node-driver-registrar: v1.1.0 => v1.3.0; and livenessprobe: v1.1.0 => livenessprobe:v2.0.0).Jira: OSD-4187