gce/upgrade.sh: Prompt if etcd version is unspecified.

[enisoc]

We shouldn't upgrade etcd without first warning the user that some etcd
version transitions can't be undone. We don't know what version the user
currently has, so we require either an explicit version and image, or an
interactive acknowledgement of this caveat.

This is modeled after the STORAGE_MEDIA_TYPE prompt just above.

@BenTheElder @krousey @wojtek-t If we do this, it would probably require all upgrade jobs to explicitly specify the etcd version, or otherwise add some variable to bypass this check. That's unfortunate coupling, but I think we need this or something like it to protect users. What do you think?

Fixes #57013

/hold
until we decide how to fix upgrade/downgrade e2e tests to account for this.

NONE

[jpbetz]

Looks right to me.

[BenTheElder]

This PR SGTM, agree that more coupling is unfortunate but it seems like if we want to force a version we should definitely set it in the job config(s). I don't think this is unreasonable at all, especially since it seems it depends on which version(s) we are upgrading between.

[BenTheElder]

/retest
garbage collector tests are so flaky :(

[wojtek-t]

/approve no-issue

[dims]

/lgtm

[enisoc]

@BenTheElder Can you help me figure out all the places we'd need to plumb in etcd versions to the test jobs, so this PR won't break them?

[BenTheElder]

@enisoc will do, it should just be jobs doing master upgrades right?

[enisoc]

@BenTheElder Thanks! Yeah, master upgrade or downgrade. The new check in this PR shouldn't trigger for node-only migrations.

[BenTheElder]

I'm pretty certain I have all the jobs that upgrade or downgrade, but I'm not sure which etcd versions they need to pin to yet, @krousey previously pinned some jobs here: kubernetes/test-infra#5909 kubernetes/test-infra#5920

[enisoc]

We have a couple options for which etcd versions to pin for each job: either the 1.8 default (etcd 3.0.17) or the 1.9 default (etcd 3.1.10).

Ideally we should also pin the etcd version during initial kube-up, so we know for sure what we're starting with.

I think these are the main scenarios we're going to recommend to users:

• upgrade 1.8 -> 1.9 (allow etcd upgrade)
  • pin to 3.0.17 to bring up 1.8 cluster
  • pin to 3.1.10 to upgrade to 1.9
• downgrade 1.9 -> 1.8 (stay on new etcd)
  • pin to 3.1.10 to bring up 1.9 cluster
  • pin to 3.1.10 to downgrade to 1.8

If we have enough jobs to cover other scenarios as well, we could do these:

• upgrade 1.8 -> 1.9 (stay on old etcd)
  • pin to 3.0.17 to bring up 1.8 cluster
  • pin to 3.0.17 to upgrade to 1.9
• downgrade 1.9 -> 1.8 (stay on old etcd)
  • pin to 3.0.17 to bring up 1.9 cluster
  • pin to 3.0.17 to downgrade to 1.8

[enisoc]

Along those lines, do the -cluster upgrade/downgrade tests also incidentally do master upgrades? Or do they only touch nodes?

[BenTheElder]

I believe they do master as well per:

kubernetes/test/e2e/upgrades/upgrade.go

Lines 36 to 38 in b983cc7

	// ClusterUpgrade indicates that both master and nodes are
	// being upgraded.
	ClusterUpgrade

(these jobs have --ginkgo.focus=\\[Feature:ClusterUpgrade\\] ...)

[BenTheElder]

Also we have upgrade/downgrade jobs for 1.6/1.7 at the moment, but we can probably safely ignore 1.6 related tests...

[enisoc]

Please take another look at the changes.

After talking with @BenTheElder, we realized that hard-coding the versions is more complex than I thought. In particular, we don't currently have the ability to separately specify the starting and ending etcd version for an upgrade. Adding this capability would require coordinating changes across both the test code and the job configs.

As a workaround, we decided instead to add a variable to let tests bypass the check, as if the user has said, "Yes, continue with the default upgrade behavior. I understand and accept the consequences." We believe this is a more realistic scenario than testing only the case where the user pins the etcd version prior to upgrading.

[BenTheElder]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, dims, enisoc, wojtek-t

Associated issue: #57013

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

Needs approval from an approver in each of these OWNERS Files:

• cluster/gce/OWNERS [wojtek-t]
• test/OWNERS [enisoc,wojtek-t]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[k8s-github-robot]

[MILESTONENOTIFIER] Milestone Pull Request Current

@BenTheElder @dims @enisoc @gmarek @vishh

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

• sig/gcp sig/release: Pull Request will be escalated to these SIGs if needed.
• priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
• kind/cleanup: Adding tests, refactoring, fixing old bugs.

Help

• Additional instructions
• Commands for setting labels

[enisoc]

/hold cancel

[xiangpengzhao]

@enisoc since this PR gets merged, should we revert kubernetes/test-infra#5909 (and kubernetes/test-infra#5920)
cc @krousey

[enisoc]

@xiangpengzhao No, we still need that fix. This PR on its own would have no effect on e2e tests (it wouldn't have fixed the problem there). This is just adding a warning for users.

[xiangpengzhao]

@enisoc thanks! I misunderstood sth here :)