Bugfix: Multi-hop etcd upgrade

[knisbet]

Description

fix a glitch in the upgrade planning for multi-hop upgrades that selects the wrong etcd base version

Type of change

• Bug fix (non-breaking change which fixes an issue)

Linked tickets and other PRs

• Closes #, refs #

• Requires #

• Ports #

TODOs

• Self-review the change
• Address review feedback

Implementation

Performance/Scaling

Testing done

Re-enabled the multi-hop upgrade integration test INTERMEDIATE_UPGRADE_MAP[5.5.10]="centos:7" and test case is now passing.

Additional information

[lenko-d]

The PR is missing vital information. Please describe the steps needed to reproduce the issue. More info about the issue and what testing was done to prove that it is working now.

[knisbet]

The PR is missing vital information. Please describe the steps needed to reproduce the issue. More info about the issue and what testing was done to prove that it is working now.

Sorry my bad, probably rushed this one a bit when the CI passed.

INTERMEDIATE_UPGRADE_MAP[5.5.10]="centos:7" is an integration test that exercises multi-hop upgrades by our integration tests. This test had been disabled due to the failure. This PR fixes the code under test and re-enables the integration test.

[a-palchikov]

I think this might be confusing overall and I'd consider moving the logic of extracting the last etcd version to a separate function:

func (r phaseBuilder) getInstalledOrUpgradedEtcdVersion() (result *semver.Version, err error) {
	if len(r.steps) != 0 {
		if lastStep := r.steps[len(r.steps)-1].etcd; lastStep != nil {
			return semver.New(lastStep.update), nil
		}
	}
	result, err = getEtcdVersionFromManifest(r.installedApp.Manifest, r.packages)
	if err != nil && !trace.IsNotFound(err) {
		return nil, trace.Wrap(err)
	}
	if result == nil {
		result = &r.currentEtcdVersion
	}
	return result, nil
}

and then:

if !r.isDirectUpgrade() {
	r.steps, err = r.buildIntermediateSteps(ctx)
	if err != nil {
		return trace.Wrap(err)
	}
}
installedOrUpgradedEtcdVersion, err := r.getInstalledOrUpgradedEtcdVersion()
// ...

Just my two cents.

[knisbet]

Yea I agree it's confusing and did consider going an approach with a separate function. My biggest worry is it creates an opportunity to break by getting out of sync with future changes to the planning logic vs just clearly returning what version was used when creating the plan at the risk of more variables returned. Although your proposed approach looks less likely to get out of sync then what I was originally considering.