Simplify update-vendor script

[benmoss]

Change our update-vendor script so we use go modules in a normal-ish way, avoiding all the /tmp staging folder stuff. I split up the commits just so that we can backport the script changes independently.

/area dependency
/area cluster-autoscaler

[k8s-ci-robot]

@benmoss: The label(s) area/cluster-autoscaler cannot be applied, because the repository doesn't have them

In response to this:

Change our update-vendor script so we use go modules in a normal-ish way, avoiding all the /tmp staging folder stuff.

I don't know if this totally eliminates our ability to vendor a specific commit of k/k, but I think it might, so that might be a blocker. We can still bump to alpha releases, as I did as part of this PR. I split up the commits just so that we can backport the script changes independently.

/area dependency
/area cluster-autoscaler

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.

[MaciekPytel]

I think every other k8s release we end up having to make some last-minute fix in scheduler that has to be cherry-picked on release branch just before the release is cut. Until we figure out a process solution to this, I think I'd prefer to at least keep the old script around so we can use it in emergency.

Maybe for now we can just have both scrips and use the new one when we can and the old one when we have to?

[elmiko]

i tried out the script with the 1.20.0-alpha.1 tag and everything worked as expected. i have one small question, but it's not a blocker for me.
/lgtm

[feiskyer]

Is it possible to make it also work for a k8s commit ID?

[MaciekPytel]

@feiskyer I don't think so. Technically I think you can have go mod target a commit hash in kubernetes/kubernetes, but the problem is with other k8s repos (ex. client-go). The source of truth for those is staging/ directory in kubernetes repo and, absent tag, it's hard to be sure which commit should be used across those repos to be in consistent state. This is exactly the reason for the old script vendoring in the contents of the staging/ directory instead of the actual content of those repos (via local commits).

[benmoss]

You can develop against it locally with replace directives (https://thewebivore.com/using-replace-in-go-mod-to-point-to-your-local-module/) but that wouldn't be something we'd want to commit

[benmoss]

We discussed on Slack some potential solutions if we need to switch back to an unreleased version of k8s:

• Keep the other update-vendor just in case, though this is messy
• Use submodules to just bump one package dependency. The downside of this is that you can create a weird mix of kube-scheduler being incompatible with another subcomponent if you aren't careful.
• Use submodules to bump all the Kubernetes dependencies together

[MaciekPytel]

I'm strongly against second option - I think it is very risky to let different k8s repos get out of sync. Also the actual scheduler logic is in kubernetes/kubernetes repo (pkg/scheduler directory mostly), I don't think there is any particular need to submodule kube-scheduler repo (which I think is just scheduler-related APIs).

The benefit of first approach is that most of the time we could ignore the existence of that option. But, if we ever have to use it, that particular version would end up working very differently from other versions (ex. you wouldn't be able to run unittests without GO111MODULE=off, etc).
Submodule option seems interesting. I'm trying to figure out if there are any non-obvious downsides to it.

@vivekbagade @towca - I think you have quite a bit of experience in this area. I wonder if you have any opinions on this?

[RainbowMango]

Indeed it's hard to get all staging repo's pseudo version by a k/k commit ID. Given we don't always depend on a k/k's commit ID, once there is a need, we can get the staging repo's pseudo version and update go.mod manually. (each staging repo merge commit message contains the k/k commit ID, e.g. kubernetes/api@2c3c141)

It's not just CA has this problem, people discussed a lot at this issue. Seems using a tagged version can solve most of the cases.

[RainbowMango]

Hey folks, I want to add a new SDK for Huawei Cloud, can we make a quick decision and push forward?

Can we merge this PR first and create a new issue for a follow-up?
If not, can we merge #3593 which just modified the go.mod file and vendor directory? so that we can keep discussing the hack/update-vendor.sh thing by current PR.

How do you think? @feiskyer @benmoss @MaciekPytel

[MaciekPytel]

Good point on kubernetes commit hashes in commit messages. Seems like it would be possible to write a script that goes through each repo and checks if the latest commit in that repo is also the last commit to a given subdirectory of staging. It seems like it may be a bit messy, but it seems like a reasonable solution for vendoring specific commit id.

I think vendoring kubernetes in a way that doesn't break go mod and vendoring provider specific dependencies are a completely separate issue though. The problem with latter is how to avoid non-core dependencies when compiling CA with just one provider or when maintaining provider in a fork (AFAIK there are still more forked providers than in-repo ones). I don't really have any good ideas for how to deal with that, but I'd be very happy to discuss any proposals.

[RainbowMango]

Seems like it would be possible to write a script that goes through each repo and checks if the latest commit in that repo is also the last commit to a given subdirectory of staging.

Yes. To be precise, the script should get commit ID from the staging repo for any Kubernetes commit, not only the latest.

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[RainbowMango]

I think we need more than one scripts, just like k/k does:

• pin-dependency.sh // pin specific version of dependency.
• update-vendor.sh // update vendor.
• verify-vendor.sh // make sure vendor has been updated.

And another script takes responsibility to retrieve commit ID from all staging repo according to k/k commit ID.
e.g. pin-kubernetes-dependency.sh which will invok pin-dependency.sh.

[MaciekPytel]

I'm not sure about pin-dependency.sh as I'm not sure if we currently have any dependency that is not already a dependency of kubernetes (in which case we probably want to use whatever version they use). Otherwise I agree.

[benmoss]

I just added the hack/submodule-k8s.sh script and tested to make sure they work together, ie that we can switch back and forth with little friction. They both will update the vendor directory so I don't think we need a separate update-vendor script unless I'm misunderstanding something.

I think verify-vendor seems nice but considering it's not trivial to add can we push that off to a separate issue?

[RainbowMango]

@benmoss
Maybe we don't need a submodule here. How do you think about the ideas mentioned by @MaciekPytel ?

Seems like it would be possible to write a script that goes through each repo and checks if the latest commit in that repo is also the last commit to a given subdirectory of staging. It seems like it may be a bit messy, but it seems like a reasonable solution for vendoring specific commit id.

I think verify-vendor seems nice but considering it's not trivial to add can we push that off to a separate issue?

SGTM. I love iteration.

[benmoss]

I don't think I understand, there's no way to have go.mod point to a specific commit. I think submoduling is the only way that works.

[MaciekPytel]

there's no way to have go.mod point to a specific commit

I think you can do it using v0.0.0-<commit_timestamp>-<commit_hash> syntax. This is mentioned in https://github.com/golang/go/wiki/Modules#can-a-module-consume-a-package-that-has-not-opted-in-to-modules and we already have some examples of that in our go.mod: https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/go.mod#L42). Or is there a problem with this approach that I'm not aware of?

[benmoss]

Ah, yeah that's true. I still don't see why it's preferable, it seems a lot more complicated than submodules.

[RainbowMango]

I still don't see why it's preferable, it seems a lot more complicated than submodules.

In my opinion, submodule way is git-ish, and we'd prefer go-ish, right?

[benmoss]

I don't think that's a fair way to contrast them. Submodules solve the problem more simply in my opinion, you point to a specific SHA of k/k and then all the staging repos will be pulled from inside of there.

With the proposed bash script solution we have to correlate the Git commit inside k/k to the commits in each of the staging repos. This isn't trivial.

Either way though, both solutions work, and I don't really know why we're debating it endlessly. If people really want to go with that approach, then by all means let's write that script and go with it. It seems like a mostly hypothetical problem at this point, considering we don't need to be on an unreleased commit of k/k right now as far as I know. I wish we could just move forward on this and iterate as we encounter problems.

[RainbowMango]

If people really want to go with that approach, then by all means let's write that script and go with it. It seems like a mostly hypothetical problem at this point, considering we don't need to be on an unreleased commit of k/k right now as far as I know. I wish we could just move forward on this and iterate as we encounter problems.

+1
@MaciekPytel How do you say?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: benmoss, elmiko
To complete the pull request process, please assign feiskyer after the PR has been reviewed.
You can assign the PR to them by writing /assign @feiskyer in a comment when ready.

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

Needs approval from an approver in each of these files:

• cluster-autoscaler/OWNERS

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

[towca]

The changes to the scripts LGTM, one question to the Makefile. The vendor update got out of sync because we had to do an update using the old script in the meantime. Could you change the PR to use a newer tag for the update, or just remove that commit altogether and only leave the script changes?

[towca]

Hey @benmoss! This is a very valuable contribution, would you consider reopening the PR and rebasing? If not, would you mind if I copied the changes and merged them myself?

[k8s-ci-robot]

@benmoss: PR needs rebase.

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.

[elmiko]

@towca i don't want to speak for @benmoss , but i know he has been focusing in other areas recently. i think if you opened a new PR with a rebase that would be fine.

[RainbowMango]

Glad to see this PR could move forward. @MaciekPytel How do you say?

[towca]

@elmiko @benmoss @RainbowMango The changes have been merged in #3915.

[RainbowMango]

Fantastic!! Thanks, @towca for your hard work.

[elmiko]

Fantastic!! Thanks, @towca for your hard work.

+1, thanks @towca !