deployapi: Refactor internal objects to match versioned #6246

0xmichalis · 2015-12-09T15:10:08Z

Forked from #6233

@deads2k the PR you asked for.

cc: @ironcladlou @liggitt

TODO:

Fix unit tests
Generate new conversions
Refactor to use new internal changes everywhere
Support only imagestreamtags for deployment ICTs (DeploymentConfig ImageChange trigger should use an object reference of ImageStreamTag and not ImageStream #2511)

Fixes #2511

0xmichalis · 2015-12-09T15:11:04Z

wip because tests need to be updated

deads2k · 2015-12-09T15:22:26Z

The usage changes look pretty straightforward.

deads2k · 2015-12-09T15:27:23Z

pkg/deploy/api/v1beta3/conversion.go

+	return nil
+}
+
+func convert_v1beta3_DeploymentConfigSpec_To_api_DeploymentConfigSpec(in *DeploymentConfigSpec, out *newer.DeploymentConfigSpec, s conversion.Scope) error {


This looks really close to what the generator would produce. Can you highlight the difference for me?

That's because I have a bachelor on conversion now. Will do.

Hm, actually all this code is here because we were doing manual conversion from the internal to out and vice versa. After running the generator though, all the conversions have been updated with these conversions. So should I drop manual? @ironcladlou ?

It's hard for me to keep up with the API conversion stuff in between the times I'm actually working on the API, so at this point I have no reason to believe the manual stuff is necessary. I'd stick with the generator by default unless a problem arises...

0xmichalis · 2015-12-09T19:14:54Z

[test]

ironcladlou · 2015-12-09T21:05:22Z

pkg/deploy/api/v1/conversion.go

@@ -143,24 +206,6 @@ func convert_v1_DeploymentTriggerImageChangeParams_To_api_DeploymentTriggerImage
 	if err := s.Convert(&in.From, &out.From, 0); err != nil {
 		return err
 	}
-	switch in.From.Kind {


Do we know for sure it's okay to stop supporting ancient triggers stored with those kinds?

The kinds are still copied as part of the object reference. This part seems to be for populating the deprecated internal fields (RepositoryName and Tag).

I am going over the generated conversions and they seem to be covering all of the manual so I'll probably remove these.

0xmichalis · 2015-12-10T14:20:43Z

@ironcladlou we currently require the name of an object (imagestream, istag) in DeploymentTriggerImageChangeParams to be a valid dns subdomain while 1) "origin-ruby-sample:latest" for example is an invalid vaue and we parse for those when converting and 2) image stream and image stream tag names have no similar kind of validation. Should I update validation?

Also do we still support imageRepository kinds?

0xmichalis · 2015-12-10T14:40:33Z

Another thing regarding validateImageChangeParams is that it's doing work that should happen when defaulting. Is this correct?

ironcladlou · 2015-12-10T15:05:50Z

@ironcladlou we currently require the name of an object (imagestream, istag) in DeploymentTriggerImageChangeParams to be a valid dns subdomain while 1) "origin-ruby-sample:latest" for example is an invalid vaue and we parse for those when converting and 2) image stream and image stream tag names have no similar kind of validation. Should I update validation?

How about reusing ValidateImageStreamName?

ironcladlou · 2015-12-10T15:06:57Z

Also do we still support imageRepository kinds?

I don't think so. @smarterclayton?

Ultimately I think conversion needs to resolve every possible trigger kind/name to an ImageStream at this point. Agree?

ironcladlou · 2015-12-10T15:07:24Z

Another thing regarding validateImageChangeParams is that it's doing work that should happen when defaulting. Is this correct?

Yeah, I think that also needs cleaned up.

smarterclayton · 2015-12-10T15:11:41Z

We do not.

On Thu, Dec 10, 2015 at 10:07 AM, Dan Mace [email protected] wrote:

Also do we still support imageRepository kinds?

I don't think so. @smarterclayton https://github.com/smarterclayton?

Ultimately I think conversion needs to resolve every possible trigger
kind/name to an ImageStream at this point. Agree?

—
Reply to this email directly or view it on GitHub
#6246 (comment).

0xmichalis · 2015-12-10T15:48:09Z

Ultimately I think conversion needs to resolve every possible trigger kind/name to an ImageStream at this point. Agree?

Right now when it's v1->api, we switch istags to is and when it's api->v1, we switch to istags. So image stream references exist only internally. Is there a reason to favor image stream references over istags? I thought the intention was to switch to the latter as buildConfigs do.

0xmichalis · 2015-12-10T16:00:14Z

How about reusing ValidateImageStreamName?

I was thinking of a combination between this and SplitImageStreamTag. Something like:

name, _, ok := SplitImageStreamTag(toBeValidated)
if !ok {
    // append error
}
if ok := ValidateImageStreamName(name); !ok {
    // append error
}

If we end up switching to istags as default. There is a comment about defaults but the actual default is different.

0xmichalis · 2015-12-10T16:02:08Z

I thought the intention was to switch to the latter as buildConfigs do.

Ref issue: #2511

ironcladlou · 2015-12-10T17:40:34Z

I was thinking of a combination between this and SplitImageStreamTag.

Sounds good to me.

0xmichalis · 2015-12-10T17:45:39Z

Right now when it's v1->api, we switch istags to is and when it's api->v1, we switch to istags. So image stream references exist only internally.

I tried two different ways to verify this on master:

Try to create a DC with an ImageChange trigger that uses an ImageStream as a kind to watch
Edit an existing DC with an ImageChange trigger to use an ImageStream as a kind to watch

In both cases I see ImageStreamTag afterwards. Went ahead and switched to use only istags - addressing #2511 as part of this api cleanup.

0xmichalis · 2015-12-11T15:53:37Z

Finally got a clean unit test run locally. This is ready for reviews

@ironcladlou @openshift/api-review

0xmichalis · 2015-12-11T17:43:45Z

Error from server: DeploymentConfig "frontend" is invalid: spec.triggers[0].imageChangeParams.from.kind: invalid value 'ImageRepository', Details: kind must be an ImageStreamTag

The image change controller for deployments never cared (prior and after this PR) about the kind of the object reference in the imageChangeParams of an ICT and since so far we allow ImageRepository references in our validation, any DC with ImageRepository set instead of {ImageStream, ImageStreamTag} is working like a charm. The validation changes in this PR break this behavior. Is this ok? Should we make oc status warn about ImageRepository references (and any kind of deprecated references in general)?

@ironcladlou @smarterclayton @deads2k @liggitt

deads2k · 2015-12-11T17:56:26Z

any DC with ImageRepository set instead of {ImageStream, ImageStreamTag} is working like a charm.

What does it actually do? Does it treat it as an ImageStream?

0xmichalis · 2015-12-11T18:00:01Z

Does it treat it as an ImageStream?

Yes.

deads2k · 2015-12-11T18:04:25Z

Seems like there's a reasonable case for silently coercing it. Why would we want to support returning the old value from a GET? Do old clients not handle both?

0xmichalis · 2015-12-14T14:31:47Z

All green here.

Two outstanding issues:

ImageStream supprot in image triggers: We currently convert any imageStream references to an istTag. Should we continue doing that?
ImageRepository support in image triggers: deployapi: Refactor internal objects to match versioned #6246 (comment) . Should we continue supporting those references? Maybe do something similar to 1. ?

ironcladlou · 2015-12-14T15:04:42Z

All green here.
Two outstanding issues:

ImageStream supprot in image triggers: We currently convert any imageStream references to an istTag. Should we continue doing that?

ImageRepository support in image triggers: deployapi: Refactor internal objects to match versioned #6246 (comment) . Should we continue supporting those references? Maybe do something similar to 1. ?

Correct me if I'm wrong but we've been storing the original Kind values (which could include ImageStream or even DockerImage) even though we do conversions on the other fields. So we do need to support dealing with the old stored Kinds, but I don't know that we need to accept anything but ImageStreamTag through the API... @deads2k @liggitt?

smarterclayton · 2015-12-14T15:18:04Z

ImageRepository kinds still have to be supported in triggers until you
write a migration that converts them to something that doesn't.

On Mon, Dec 14, 2015 at 9:31 AM, Michail Kargakis [email protected]
wrote:

All green here.

Two outstanding issues:

ImageStream supprot in image triggers: We currently convert any
imageStream references to an istTag. Should we continue doing that?

ImageRepository support in image triggers: deployapi: Refactor internal objects to match versioned #6246 (comment)
deployapi: Refactor internal objects to match versioned #6246 (comment) .
Should we continue supporting those references? Maybe do something similar
to 1. ?

—
Reply to this email directly or view it on GitHub
#6246 (comment).

0xmichalis · 2015-12-18T11:27:34Z

@deads2k g.Edges() seems to return the edges non-deterministically. We didn't hit it before either in TestDCPodTemplateSpecNode (there was only one edge returned) or elsewhere (in every test we use it, we just check for the length of the set or the kind of the edges which always is the same for the tests we are running). Now that there are two edges in TestDCPodTemplateSpecNode and we inspect them, I scarcely see the second edge being returned first.

func TestDCPodTemplateSpecNode(t *testing.T) {
    // other stuff
    edges := g.Edges()
    // other stuff
    if edges[0].From().ID() != dcNode.ID() {
        t.Errorf("expected %v (id: %d), got %v (id: %d)", dcNode, dcNode.ID(), edges[0].From(), edges[0].From().ID())
        t.Errorf("EDGES: %v", edges)
    }
    podTemplateNode := edges[0].To()
    if _, ok := podTemplateNode.(*kubetypes.PodTemplateSpecNode); !ok {
        t.Errorf("expected a PodTemplate from the DC node")
        t.Errorf("got %v (id: %d)", edges[0].To(), edges[0].To().ID())
    }
    podSpecNode := edges[1].To()
    if _, ok := podSpecNode.(*kubetypes.PodSpecNode); !ok {
        t.Errorf("expected a PodSpec from the PodTemplate node")
        t.Errorf("got %v (id: %d)", edges[1].To(), edges[1].To().ID())
    }
}

--- FAIL: TestDCPodTemplateSpecNode (0.00s)
    nodes_test.go:40: expected DeploymentConfig|ns/foo (id: 1), got PodTemplateSpec|DeploymentConfig|ns/foo (id: 2)
    nodes_test.go:41: EDGES: [{{{PodTemplateSpec|DeploymentConfig|ns/foo PodSpec|PodTemplateSpec|DeploymentConfig|ns/foo} map[Contains:{}]} 1} {{{DeploymentConfig|ns/foo PodTemplateSpec|DeploymentConfig|ns/foo} map[Contains:{}]} 1}]
    nodes_test.go:44: expected a PodTemplate from the DC node
    nodes_test.go:45: got PodSpec|PodTemplateSpec|DeploymentConfig|ns/foo (id: 3)
    nodes_test.go:49: expected a PodSpec from the PodTemplate node
    nodes_test.go:50: got PodTemplateSpec|DeploymentConfig|ns/foo (id: 2)
FAIL
FAIL    github.com/openshift/origin/pkg/deploy/graph/nodes  0.005s

0xmichalis · 2015-12-18T11:36:18Z

I opened #6394 and will refactor the test to just check for the nodes for the time being.

0xmichalis · 2015-12-18T12:34:20Z

ERROR: gave up waiting for https://127.0.0.1:28443/healthz

!!! Error in hack/test-cmd.sh:364
    'return 1' exited with status 1
Call stack:
    1: hack/test-cmd.sh:364 main(...)
Exiting with status 1

[test]

deads2k · 2015-12-18T13:11:56Z

I opened #6394 and will refactor the test to just check for the nodes for the time being.

Two "normal" choices:

run a topo sort and compare against expected.
walk the nodes from your expected root like: https://github.com/openshift/origin/blob/master/pkg/api/kubegraph/nodes/nodes_test.go#L56

0xmichalis · 2015-12-18T13:20:38Z

run a topo sort and compare against expected.

Nice! That topo package is golden. Updated the graph test, PTAL

deads2k · 2015-12-18T13:24:21Z

Nice! That topo package is golden. Updated the graph test, PTAL

lgtm, squash it in for merge.

0xmichalis · 2015-12-18T13:25:14Z

ready

deads2k · 2015-12-18T13:30:26Z

lgtm [merge]

0xmichalis · 2015-12-19T05:35:33Z

flake #6419

[merge]

0xmichalis · 2015-12-19T06:55:52Z

imagestreamtag ❄️

0xmichalis · 2015-12-19T14:13:17Z

[merge]

0xmichalis · 2015-12-19T15:36:36Z

oc status broken test #6423

fix in #6425

0xmichalis · 2015-12-21T08:59:21Z

[merge]

0xmichalis · 2015-12-21T11:05:32Z

flake #6065

re[merge]

0xmichalis · 2015-12-21T13:13:39Z

flake #6065

again?

👻 [merge] 👻

0xmichalis · 2015-12-21T15:25:35Z

ERROR: gave up waiting for http://127.0.0.1:44001/version

!!! Error in hack/test-integration.sh:364
    'return 1' exited with status 1
Call stack:
    1: hack/test-integration.sh:364 main(...)
Exiting with status 1
make: *** [test-int-plus] Error 1

[merge]

0xmichalis · 2015-12-22T09:03:04Z

ui flake

[merge]

0xmichalis · 2015-12-22T10:10:59Z

https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4453/
#6447 (comment) flake

[merge]

openshift-bot · 2015-12-22T12:10:20Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4458/) (Image: devenv-rhel7_3020)

0xmichalis · 2015-12-22T12:17:54Z

#6149 flake

0xmichalis · 2015-12-22T12:17:59Z

[test]

openshift-bot · 2015-12-22T12:20:17Z

Evaluated for origin test up to 05e2052

openshift-bot · 2015-12-22T13:21:04Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8065/)

0xmichalis · 2015-12-22T13:22:46Z

Tests passed at once but this still won't [merge] :')

openshift-bot · 2015-12-22T13:25:16Z

Evaluated for origin merge up to 05e2052

Merged by openshift-bot

0xmichalis changed the title ~~deployapi: Refactor internal object to match versioned~~ [WIP] deployapi: Refactor internal object to match versioned Dec 9, 2015

0xmichalis changed the title ~~[WIP] deployapi: Refactor internal object to match versioned~~ [WIP] deployapi: Refactor internal objects to match versioned Dec 9, 2015

deads2k reviewed Dec 9, 2015
View reviewed changes

ironcladlou reviewed Dec 9, 2015
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2015

0xmichalis mentioned this pull request Dec 11, 2015

Enhance status for deploymentconfigs #6233

Merged

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2015

0xmichalis mentioned this pull request Dec 18, 2015

gonum/graph.Edges() is non-deterministic #6394

Closed

deployapi: Necessary refactoring after updating the internal objects

05e2052

openshift-bot pushed a commit that referenced this pull request Dec 22, 2015

Merge pull request #6246 from kargakis/refactor-internal-dc-object

5e2cd8f

Merged by openshift-bot

openshift-bot merged commit 5e2cd8f into openshift:master Dec 22, 2015

0xmichalis deleted the refactor-internal-dc-object branch December 22, 2015 15:20

deployapi: Refactor internal objects to match versioned #6246

deployapi: Refactor internal objects to match versioned #6246

Conversation

0xmichalis commented Dec 9, 2015

0xmichalis commented Dec 9, 2015

deads2k commented Dec 9, 2015

deads2k Dec 9, 2015

Choose a reason for hiding this comment

0xmichalis Dec 9, 2015

Choose a reason for hiding this comment

0xmichalis Dec 9, 2015

Choose a reason for hiding this comment

ironcladlou Dec 9, 2015

Choose a reason for hiding this comment

0xmichalis commented Dec 9, 2015

ironcladlou Dec 9, 2015

Choose a reason for hiding this comment

0xmichalis Dec 10, 2015

Choose a reason for hiding this comment

0xmichalis Dec 10, 2015

Choose a reason for hiding this comment

0xmichalis commented Dec 10, 2015

0xmichalis commented Dec 10, 2015

ironcladlou commented Dec 10, 2015

ironcladlou commented Dec 10, 2015

ironcladlou commented Dec 10, 2015

smarterclayton commented Dec 10, 2015

0xmichalis commented Dec 10, 2015

0xmichalis commented Dec 10, 2015

0xmichalis commented Dec 10, 2015

ironcladlou commented Dec 10, 2015

0xmichalis commented Dec 10, 2015

0xmichalis commented Dec 11, 2015

0xmichalis commented Dec 11, 2015

deads2k commented Dec 11, 2015

0xmichalis commented Dec 11, 2015

deads2k commented Dec 11, 2015

0xmichalis commented Dec 14, 2015

ironcladlou commented Dec 14, 2015

smarterclayton commented Dec 14, 2015

0xmichalis commented Dec 18, 2015

0xmichalis commented Dec 18, 2015

0xmichalis commented Dec 18, 2015

deads2k commented Dec 18, 2015

0xmichalis commented Dec 18, 2015

deads2k commented Dec 18, 2015

0xmichalis commented Dec 18, 2015

deads2k commented Dec 18, 2015

0xmichalis commented Dec 19, 2015

0xmichalis commented Dec 19, 2015

0xmichalis commented Dec 19, 2015

0xmichalis commented Dec 19, 2015

0xmichalis commented Dec 21, 2015

0xmichalis commented Dec 21, 2015

0xmichalis commented Dec 21, 2015

0xmichalis commented Dec 21, 2015

0xmichalis commented Dec 22, 2015

0xmichalis commented Dec 22, 2015

openshift-bot commented Dec 22, 2015

0xmichalis commented Dec 22, 2015

0xmichalis commented Dec 22, 2015

openshift-bot commented Dec 22, 2015

openshift-bot commented Dec 22, 2015

0xmichalis commented Dec 22, 2015

openshift-bot commented Dec 22, 2015