-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Preserve namespace on imagestreams server-side export #18487
Preserve namespace on imagestreams server-side export #18487
Conversation
@@ -1199,9 +1199,6 @@ func (e *Store) calculateTTL(obj runtime.Object, defaultTTL int64, update bool) | |||
// present when the object is exported. | |||
func exportObjectMeta(accessor metav1.Object, exact bool) { | |||
accessor.SetUID("") | |||
if !exact { | |||
accessor.SetNamespace("") |
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.
@deads2k from what I can tell, it wasn't the export strategy that was defaulting an empty namespace to "default".
In the case of the imagestream I tested with, its ExportStrategy
was nil. Instead, the CreateStrategy
was called here. I suspect it is somewhere there that the namespace is defaulted
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 suspect the client to set the namespace wrong on export. The "default" namespace is not special and I guess whoever reported that BZ was just using "default" as the current namespace.
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.
This code looks correct. Check the return value from teh apiserver. Was it actually "default" or was that set client-side?
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.
Taking a second look, this problem does not exist when testing against a cluster created with openshift start
(compiled from latest master).
Can only replicate if using a cluster created via oc cluster up
. Still looking
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.
Was it actually "default" or was that set client-side?
It does not look like the client is setting it to "default", from what I can tell. Namespace seems to be set already when the client receives a response from the server.
--loglevel=8 of oc get is/jenkins -o yaml --export
: http://pastebin.test.redhat.com/553666
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.
@mfojtik @deads2k for comparison, here is --loglevel 8
output for the same command, but executed against a cluster created via openshift start
instead (same result with an openshift binary compiled from both latest master, and tag v3.9.0-alpha.4, which is the image version I am using for oc cluster up
): http://pastebin.test.redhat.com/553669
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.
cc @bparees I'm not sure if oc cluster up
uses a different export strategy / why it appears to behave differently than an $ openshift start
cluster.
We handle server-side export here: https://github.com/openshift/origin/blob/master/vendor/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go#L1215 clearing the namespace field for an object here https://github.com/openshift/origin/blob/master/vendor/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go#L1203
Not sure how / where an empty namespace field could be defaulting to default
as its value. The client does not appear to be altering this, as loglevel output linked to above shows that the cluster is already returning the object with the namespace field value defaulted.
This does not happen with openshift start
clusters. I have tested with 3.8 and 3.9 releases of the openshift binary.
I have also tested with oc cluster up --latest
and oc cluster up --version v3.8.0-alpha.0
. Was hoping you had maybe seen something like this before?
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.
Was hoping you had maybe seen something like this before?
i haven't, and no, oc cluster up certainly doesn't configure export strategies, so i can't imagine why there would be a difference in behavior.
also "oc export" seems to do the right thing (leaves off the namespace).
Anyway I'm seeing the "wrong" behavior using openshift start clusters, as well as oc cluster up clusters:
exporting from current project==openshift:
$ oc get --export is jenkins -o yaml
apiVersion: image.openshift.io/v1
kind: ImageStream
metadata:
annotations:
openshift.io/display-name: Jenkins
openshift.io/image.dockerRepositoryCheck: 2018-02-07T19:19:34Z
creationTimestamp: null
generation: 1
name: jenkins
namespace: default
/retest |
/hold Please have upstream PR approved first and use the upstream PR number in commit message. |
/hold fix doesn't look correct. |
/lgtm cancel |
My current namespace was "logging" not "default" in latest repro in https://bugzilla.redhat.com/show_bug.cgi?id=1542238 |
4482be4
to
ec47625
Compare
ec47625
to
f38e47e
Compare
@mfojtik @soltysh @deads2k Updated this PR to just add an export strategy for imagestreams. Before, they did not have one, so doing a server-side export was defaulting to a call to the imagestream create-strategy. This strategy was defaulting the namespace to the "Default" value when it saw that the namespace field had been cleared. ptal |
lgtm for whatever that's worth. |
I've talked with @juanvallejo on IRC he'll try to fix setting the namespace in |
/assign |
ff35077
to
c7e6544
Compare
@soltysh thanks for the feedback. Added an extra arg to |
@juanvallejo update tests:
and you're good to go. |
/hold cancel |
c7e6544
to
2074572
Compare
2074572
to
f07954b
Compare
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: juanvallejo, mfojtik, soltysh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test gcp |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue (batch tested with PRs 18487, 18537, 18630). |
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1542238
cc @deads2k