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.

Sign up for GitHub

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

Jump to bottom

Update kOps to v1.23.0 + Update parameters.md #1329

Merged
merged 3 commits into from
Aug 11, 2022
Merged

Update kOps to v1.23.0 + Update parameters.md #1329

merged 3 commits into from
Aug 11, 2022

Conversation

torredil
Copy link
Member

@torredil torredil commented Jul 29, 2022 •
edited
Loading

What is this PR about? / Why do we need it?

What testing is done?

  • CI

closes #1187

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 29, 2022
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 29, 2022
@torredil
Copy link
Member Author

/test all

@torredil
Copy link
Member Author

/retest

@torredil torredil force-pushed the master branch 3 times, most recently from 5b84f62 to affb01f Compare July 29, 2022 19:25
@torredil torredil marked this pull request as ready for review July 29, 2022 19:25
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2022
@k8s-ci-robot k8s-ci-robot requested a review from wongma7 July 29, 2022 19:26
@torredil
Copy link
Member Author

Sorry for the noise, sniped by flaky CI and fixing co-author info in commit msg.
PTAL @wongma7

@wongma7
Copy link
Contributor

wongma7 commented Jul 29, 2022

/lgtm

I think the pull-aws-ebs-csi-driver-migration-test failures/flakes are because of bug in the tests, we might consider skipping all 'pre-provisioned' test cases since 'dynamic' test cases are basically a superset of those.
Jul 29 17:38:56.144: INFO: At 2022-07-29 17:33:45 +0000 UTC - event for exec-volume-test-preprovisionedpv-wg7c: {attachdetach-controller } FailedAttachVolume: AttachVolume.Attach failed for volume "aws-444x6" : rpc error: code = Internal desc = Could not attach volume "vol-01c6fe3ef04e529d3" to node "i-03497c820bd31d419": could not attach volume "vol-01c6fe3ef04e529d3" to node "i-03497c820bd31d419": InvalidVolume.ZoneMismatch: The volume 'vol-01c6fe3ef04e529d3' is not in the same availability zone as instance 'i-03497c820bd31d419'

for the non-migration test suites, CSI topology ensures this problem never happens. But for the pull-aws-ebs-csi-driver-migration-test suite, the pre-provisioned tests naively create the PV without any zone/topology label, it should add the label :
kubernetes/kubernetes#108696

What I dont understand is how we are the only ones hitting this failure , it seems like it should also affect GCE PD test cases , anyway i would selfishly like kubernetes/kubernetes#108696 to merge anyway.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2022
wongma7
wongma7 reviewed Jul 29, 2022
View reviewed changes
hack/kops-patch.yaml
@@ -172,3 +172,6 @@ spec:
ExpandCSIVolumes: "true"
CSIInlineVolume: "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW when bumping K8S_VERSION we can remove CSIDriverRegistry, CSINodeInfo, and CSIBlockVolume, all of these are GA since 1.18 so no need to explicitly set true

Copy link
Member Author

Choose a reason for hiding this comment

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

ack, will do 👍

@torredil
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2022
torredil and others added 2 commits August 3, 2022 09:55
@torredil @wongma7 @ConnorJC3
Update to kOps v1.23.0
371a36a 
Co-authored-by: wongma7 <mattwon@amazon.com>
Co-authored-by: ConnorJC3 <github@connorjc.io>
Signed-off-by: Eddie Torres <torredil@amazon.com>
@torredil @ConnorJC3
Update parameters.md
4236d8d 
Co-authored-by: ConnorJC3 <github@connorjc.io>
Signed-off-by: Eddie Torres <torredil@amazon.com>
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 3, 2022
@kubernetes-sigs kubernetes-sigs deleted a comment from k8s-ci-robot Aug 3, 2022
@rdpsin
Copy link
Contributor

rdpsin commented Aug 3, 2022

/retest

1 similar comment
@torredil
Copy link
Member Author

torredil commented Aug 3, 2022

/retest

@torredil
Copy link
Member Author

torredil commented Aug 3, 2022

Most recent CI was failing due to overriding the GINKGO_SKIP value in run.sh. Latest commit accounts for this by also skipping "\[Disruptive\]" tests.

ConnorJC3
ConnorJC3 approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@ConnorJC3 ConnorJC3 left a comment

Choose a reason for hiding this comment

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

/lgtm some minor nitpicks

hack/e2e/run.sh Show resolved Hide resolved
hack/kops-patch.yaml Outdated
@@ -172,3 +172,6 @@ spec:
ExpandCSIVolumes: "true"
CSIInlineVolume: "true"
anonymousAuth: false
cloudConfig:
awsEBSCSIDriver:
enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: removed newline at end of file

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@torredil
Skip Testpattern: Pre-provisioned PV in migration tests
05b7165 
Signed-off-by: Eddie Torres <torredil@amazon.com>
@rdpsin
Copy link
Contributor

rdpsin commented Aug 8, 2022

/lgtm
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2022
@rdpsin
Copy link
Contributor

rdpsin commented Aug 11, 2022

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ConnorJC3, rdpsin, torredil

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@torredil torredil removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit 1216f44 into kubernetes-sigs:master Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdpsin rdpsin rdpsin left review comments

@wongma7 wongma7 wongma7 left review comments

@ConnorJC3 ConnorJC3 ConnorJC3 approved these changes

@gnufied gnufied Awaiting requested review from gnufied

@leakingtapan leakingtapan Awaiting requested review from leakingtapan

Assignees

@wongma7 wongma7

@rdpsin rdpsin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@torredil @k8s-ci-robot @wongma7 @rdpsin @ConnorJC3