-
Notifications
You must be signed in to change notification settings - Fork 82
Bump vendored code to match that of kubernetes-v1.10.0-alpha.3 #183
Bump vendored code to match that of kubernetes-v1.10.0-alpha.3 #183
Conversation
f9c8354
to
9606128
Compare
94dcf26
to
effbdfb
Compare
@marun, @irfanurrehman, All known issues have been fixed and tests are green. PTAL. Thanks ! |
glide.yaml
Outdated
- package: k8s.io/metrics | ||
version: kubernetes-1.9.2-beta.0 | ||
- package: k8s.io/utils | ||
version: aedf551cdb8b0119df3a19c65fde413a13b34997 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this autofilled by glide? I see that some k8s.io repos still don't have tags mapping to a particular k8s release tag (perhaps not required). But I do not clearly understand how the compatibility mapping is maintained for these as of now. Does it make sense to have any discussion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version is not auto-filled by glide. It is manually added by looking into Godeps.Json from kubernetes repo for the specific version we are bumping. Most of the repos under kubernetes are tagged now for the release. Going forward we can write a simple tool to pick up the versions and populate the glide.yaml file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why are you looking to manually control the version of dependencies from k8s rather than let glide pick them up from the godeps.json file? Glide walks the tree and when it finds a dependency it uses the information picked up along the way on dependency versions. If k8s lists a dependency revision in the godeps.json file before others list it in their metadata files it will use that version.
Imagine the case where you manually manage the version and k8s changes. Then you'll be out of sync with k8s on the version to use.
There are plenty of reasons to manually manage a dependency. I'm curious of the reason here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattfarina, federation also imports some third party packages which might be imported by k8s version and if glide resorts to using latest version of the code for such packages. it might cause some incompatibility. ( I should admit i don't know how glide handles this scenario). To ensure that we are vendoring the exact same version of the package which kubernetes version uses, i resorted to explicitly specifying version in the package. kubernetes code makes up of the bulk of vendored code what we use. and we always rely on a particular kubernetes version (which is stable enough).
IMHO, In our case, Its just a bit of overhead to change all the version of the package which we vendor when we bump.
I would also do some experiments in glide and confirm, if we can do away not mentioning version for every package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have kubernetes first in your glide.yaml than the dependency versions listed in the Kubernetes Godeps.json file will be used if no version is specified in the glide.yaml file. This is because Glide will start at the first one and iterate over the dependencies first to last in the glide.yaml file. When it picks up a version the first one wins (unless there are some semver ranges it can munge together which isn't the case with Godeps).
The ordering in Glide was an intentional decision to give developers some control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this helpful tip @mattfarina. i will update accordingly.
glide.yaml
Outdated
- pkg/watch | ||
- package: k8s.io/apiserver | ||
version: kubernetes-1.9.2-beta.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its amazing that we now are at a point where a mapping tag is available for staging repos!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, most of the repos in kubernetes are tagged with release version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release versions have always been available, but my understanding is that federation development needs to be able to chase master to ensure compatibility over a given release cycle. Has that thinking changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did not get why federation development needs to chase kubernetes master. we should ideally be jumping from one stable version to another stable version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stable release versions of federation should absolutely target stable release versions of kubernetes. I'm not sure that would be ideal for master, though. The risk of compatibility problems being introduced between federation and kubernetes during development of a release is greater if federation development only targets stable release versions vs updating a pin against master on a regular basis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially agree with your concern, but we need to consider that we are consumers of the kubernetes code and it would make sense for us to vendor in stable code as opposed to under development code. In worse cases if something breaks because of compatibility issue we can resort to bumping to the particular commit which can fix the issue. So, IMHO, the bumping strategy should be from one stable release to another stable release.
Also if you think practically, we didn't face any such issues in past couple of months, given that our federated clusters in ci jobs ran with latest master k8s code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were many problems discovered by chasing trunk in previous releases, so I'm not sure a lack of recent evidence is justification for not doing so going forward. The danger in not discovering problems as soon as possible is that the code introducing breakage will be 'baked in' and more difficult to back out or change. The strategy for minimizing disruption to the federation development effort would be to only update to a new version of k/k and its dependencies once CI had validated its stability. Only periodic jobs that validate against k/k HEAD would ever be broken, minimizing disruption for federation developers.
That said, I'm not responsible for supporting the current codebase (v1) so do what you think is best. For v2 (which should not depend on k/k), I will advocate strongly for keeping vendored staging repos as close to master as possible.
glide.yaml
Outdated
@@ -1,15 +1,19 @@ | |||
package: k8s.io/federation | |||
import: | |||
- package: cloud.google.com/go | |||
version: 3b1ae45394a234c385be014e9a488f2bb6eef821 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason that we need to add the specific hash for this and many other repos? Probably @marun can comment with his experience of using glide earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just added the version to be extra careful to use the same version of the package used by corresponding kubernetes version. if we don't use version and not pinned to the same version used by kubernetes, there are possibilities that we might use the latest version of the package which might have syntactical or semantic changes which may cause problems for our build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming glide should be able to specify the versions automatically? I hope that it isn't necessary to hand-curate the versions of vendored dependencies, that seems onerous and error-prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think, glide does not fill the version automatically and always picks the latest commits in each package. It would be foolproof, if we pin the dependent package version to that of what is used in kubernetes version. There will be no compatibility issues and also kubernetes undergoes lot more testing than federation code. I agree that it is error-prone to hand-curate the versions, but its a one time activity and some 10-20 packages that we need to fill it by looking in Godeps.json file from corresponding kubernetes version.
test/integration/framework/api.go
Outdated
return true, nil | ||
}) | ||
if err != nil { | ||
return fmt.Errorf("Timed out waiting for the %s: %v", apiNoun, err) | ||
} | ||
|
||
runOptions.InsecureServing.BindPort = port | ||
runOptions.SecureServing.ServerCert.CertDirectory = "/tmp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be created if /tmp
does not exist or not mounted? os
packages TmpDir()
can be used or taken as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no "/tmp" will not be created but only used here. This is kind of test fixture, /tmp will be available on most root file systems and is meant for temporary usage which is world writable. I just did the simplest thing necessary, to fix the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's desirable to use /tmp here, since tests run in parallel may step on one another. A new temp directory as Irfan suggests would be preferable, and that directory should be cleaned up on teardown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okie, updated.
Thanks @shashidharatd for this. I have few minor comments, otherwise looks good to me. @marun please have a look at this, given your first hand experience at initial vendoring. |
@marun, will u be able to have a quick look at this ? |
effbdfb
to
b3d3f7c
Compare
35f157d
to
55a4bc7
Compare
55a4bc7
to
33ead9c
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shashidharatd 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 OWNERS Files:
Approvers can indicate their approval by writing |
33ead9c
to
9359675
Compare
9359675
to
4caf45b
Compare
/test pull-federation-e2e-gce |
Closing this in favour of #237 |
1.9.2-beta.01.9.2-beta.0and fix the usage in federation code/cc @kubernetes/sig-multicluster-pr-reviews @irfanurrehman @marun