add imagestream refs for custom and docker strategies

[bparees]

No description provided.

[bparees]

[test]

[bparees]

@smarterclayton so i'm adding objectrefs for imagestreams to docker and custom strategies (to align with STI strategy). i know we talked about using imagestreamtags for all this at some point...do you have a problem proceeding with this for now and coming back around to use imagestreamtags?

alternatively, are we in a position where it makes sense for me to switch all this to imagestreamtags? is it a large effort? (i'm hoping to jam this change in for beta3)

[ncdc]

It probably would make sense to do this now. We should probably take this code, clean it up a bit so it's more usable to the outside, and export it so you can use it https://github.com/openshift/origin/blob/master/pkg/image/registry/imagestream/strategy.go#L191-L204

[ncdc]

The rationale being why make 2 changes if you can only make 1 instead?

[bparees]

@ncdc yeah the only reason i'd consider doing this now and that later is to get it in for beta3. if imagestreamtag is easy enough, i'm fine with doing it all in one shot.

[ncdc]

If you do ImageStreamTag, you'll have something like

From: &kapi.ObjectReference{
  Kind: "ImageStreamTag",
  Namespace: "optional",
  Name: "mycoolstream:latest",
}

You'll have to look up the stream, then call imageapi.LatestTaggedImage(stream, tag)

[bparees]

sounds easy enough :)

[bparees]

@ncdc ok I added the resolver and unit tests around it. I agree it could be in a more universal helper location, but we can refactor that later if you're ok with the implementation+tests.

@smarterclayton added the conversion we talked about, ptal.

[bparees]

Realized image change triggers probably need an update for this too, given how we currently do matching.

[bparees]

@ncdc is it even valid to watch for changes to an ImageStreamTag or an ImageStreamImage?

If not, I think ICTs will need to stay as ImageStream references, but it makes the image change trigger story a bit ugly.

[smarterclayton]

Hrm... It should be possible to watch those.

On Apr 10, 2015, at 11:48 PM, Ben Parees notifications@github.com wrote:

@ncdc is it even valid to watch for changes to an ImageStreamTag or an ImageStreamImage?

If not, I think ICTs will need to stay as ImageStream references, but it makes the image change trigger story a bit ugly.

—
Reply to this email directly or view it on GitHub.

[ncdc]

I'll have to add support, but yeah

Sent from my iPhone

On Apr 10, 2015, at 11:59 PM, Clayton Coleman notifications@github.com wrote:

Hrm... It should be possible to watch those.

On Apr 10, 2015, at 11:48 PM, Ben Parees notifications@github.com wrote:

@ncdc is it even valid to watch for changes to an ImageStreamTag or an ImageStreamImage?

If not, I think ICTs will need to stay as ImageStream references, but it makes the image change trigger story a bit ugly.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

[ncdc]

Even without direct support, you could still watch the stream, get the latest event for the tag, and compare docker image refs.

Sent from my iPhone

On Apr 10, 2015, at 11:59 PM, Clayton Coleman notifications@github.com wrote:

Hrm... It should be possible to watch those.

On Apr 10, 2015, at 11:48 PM, Ben Parees notifications@github.com wrote:

@ncdc is it even valid to watch for changes to an ImageStreamTag or an ImageStreamImage?

If not, I think ICTs will need to stay as ImageStream references, but it makes the image change trigger story a bit ugly.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

[ncdc]

And ImageStreamImage is immutable, so it only makes sense to watch the tag

Sent from my iPhone

On Apr 10, 2015, at 11:59 PM, Clayton Coleman notifications@github.com wrote:

Hrm... It should be possible to watch those.

On Apr 10, 2015, at 11:48 PM, Ben Parees notifications@github.com wrote:

@ncdc is it even valid to watch for changes to an ImageStreamTag or an ImageStreamImage?

If not, I think ICTs will need to stay as ImageStream references, but it makes the image change trigger story a bit ugly.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

[bparees]

Yeah there's some logic in how ICTs work that makes that uglier than it should be (we do some name matching so comparing an imagestream tag ref to an imagestream ref requires extra hoops and might lead us to do the wrong thing.)  I'll summarize the situation later. 

[bparees]

ok I thought about it some more and I think we can compare ImageStream[Tag] to ImageStream[Tag] after all.. just requires that if we're comparing an ImageStream to an ImageStreamTag, we first append :latest or :[Tag field value] to the ImageStream name before comparing them.

With that logic in place, and assuming we can watch ImageStreamTags along with the current logic to watch ImageStreams, i think we can retain the ICT logic for the most part. Which means we'll continue to trigger builds anytime the thing in your ICT changes, and the build that is triggered may or may not actually use the new image, depending on whether the BuildConfig Strategy From reference actually matches the From of the ICT that fired.

[smarterclayton]

The nice thing here is that the watch on tag/image can be short circuited to be more efficient.

On Apr 12, 2015, at 12:54 PM, Ben Parees notifications@github.com wrote:

ok I thought about it some more and I think we can compare ImageStream[Tag] to ImageStream[Tag] after all.. just requires that if we're comparing an ImageStream to an ImageStreamTag, we first append :latest or :[Tag field value] to the ImageStream name before comparing them.

With that logic in place, and assuming we can watch ImageStreamTags along with the current logic to watch ImageStreams, i think we can retain the ICT logic for the most part. Which means we'll continue to trigger builds anytime the thing in your ICT changes, and the build that is triggered may or may not actually use the new image, depending on whether the BuildConfig Strategy From reference actually matches the From of the ICT that fired.

—
Reply to this email directly or view it on GitHub.

[ncdc]

@bparees do you need me to add watch support for ImageStreamTags, or can you live with just watching the stream?

[smarterclayton]

I think we need watch support on ImageStreamTags

----- Original Message -----

@bparees do you need me to add watch support for ImageStreamTags, or can you
live with just watching the stream?

---

Reply to this email directly or view it on GitHub:
#1693 (comment)

[ncdc]

Ok, will do.

[bparees]

ok I think this is ready for a look again. i'm punting on handling ICTs for ImageStreamTags for now, but the Strategies do support ImageStream, ImageStreamTag, and ImageStreamImage. And you can have an ICT defined in terms of an ImageStream along with a strategy defined in terms of an ImageStreamTag and things should work.

to be honest it's become a bit of a mess to maintain and there's an argument for doing away with all the matching logic and just saying "if you request an ICT, when that ICT fires you get built using that image, regardless of what From/Image/etc your Strategy specified" but I'm still concerned that's giving up a valuable use case. I suppose that use case could be mostly met via webhooks though ("run this build whenever image foo changes, even though this build doesn't directly consume foo")

[bparees]

@smarterclayton this is failing the serialization_test because it fails roundtripping... we start out with a from reference with no Kind (meaning ImageStream) and return a from reference with a Kind of ImageStreamTag.

that's intentional behavior on the part of my converter because we switch to the internal representation of ImageStreamTag and then we can't really know to go back to ImageStream (and for that matter it could have been an ImageRepository reference too, i think...pretty sure it all gets munged into ImageStreamTag)

am I doing conversion wrong here, or do we need to be more lenient in our roundtrip tests?

[smarterclayton]

In your fuzz you just need to constrain the initial state to one that exactly round trips, and make sure in your individual conversion test you verify the round trip is correct for the other values (we don't have any clever "semantic equal" for fuzzing yet, although we should).

On Apr 21, 2015, at 11:54 PM, Ben Parees notifications@github.com wrote:

@smarterclayton this is failing the serialization_test because it fails roundtripping... we start out with a from reference with no Kind (meaning ImageStream) and return a from reference with a Kind of ImageStreamTag.

that's intentional behavior on the part of my converter because we switch to the internal representation of ImageStreamTag and then we can't really know to go back to ImageStream (and for that matter it could have been an ImageRepository reference too, i think...pretty sure it all gets munged into ImageStreamTag)

am I doing conversion wrong here, or do we need to be more lenient in our roundtrip tests?

—
Reply to this email directly or view it on GitHub.

[bparees]

[test]

[bparees]

@smarterclayton @ncdc jenkins passing finally so i think this is ready for a final review.

@Kargakis please take a look at the buildchain changes. I think i simplified it but i may have missed something.

[0xmichalis]

I would argue that it's better to extract our streams from the build configurations instead of iterating over all available namespaces for two reasons: 1) since we build dep trees based on ICTs, buildConfigs is where we want to look for streams, plus we might end up with more streams in our results than needed (can we have an imageStream around w/o a respective buildConfig?). 2) Searching through buildConfigs seems cheaper (no REST calls) vs GET streams for every namespace. Anyway, if you follow what you are doing right now, you should probably remove getRepos since it won't be used anymore.

[ncdc]

Image streams definitely can exist by themselves.

Sent from my iPhone

On Apr 23, 2015, at 4:46 AM, Michail Kargakis notifications@github.com wrote:

In pkg/cmd/experimental/buildchain/buildchain.go:

@@ -186,14 +189,24 @@ func RunBuildChain(f *clientcmd.Factory, cmd *cobra.Command, args []string) erro
// Set the specified repo as the only one to output dependencies for
repos[repo] = tags
} else {

•   // Get all image repositories from build configurations
  

- repos = getRepos(buildConfigList)

•   // Make sure from now on that the --all flag is true
  

•   // since we are building dependency trees for every
  

•   // image repository available either in the current
  

•   // namespace or in every namespace
  

•   all = true
  

•   // all the tags of all the imagestreams across all the namespaces
  

•   for _, ns := range namespaces {
  

  I would argue that it's better to extract our streams from the build configurations instead of iterating over all available namespaces for two reasons: 1) since we build dep trees based on ICTs, buildConfigs is where we want to look for streams, plus we might end up with more streams in our results than needed (can we have an imageStream around w/o a respective buildConfig?). 2) Searching through buildConfigs seems cheaper (no REST calls) vs GET streams for every namespace. Anyway, if you follow what you are doing right now, you should probably remove getRepos since it's won't be used anymore.

—
Reply to this email directly or view it on GitHub.

[smarterclayton]

N calls to the server is going to be slower than 1 in almost every case (even per namespace). The overhead for network is too high.

----- Original Message -----

@@ -186,14 +189,24 @@ func RunBuildChain(f *clientcmd.Factory, cmd
*cobra.Command, args []string) erro
// Set the specified repo as the only one to output dependencies for
repos[repo] = tags
} else {

•   // Get all image repositories from build configurations
  

- repos = getRepos(buildConfigList)

•   // Make sure from now on that the --all flag is true
  

•   // since we are building dependency trees for every
  

•   // image repository available either in the current
  

•   // namespace or in every namespace
  

•   all = true
  

•   // all the tags of all the imagestreams across all the namespaces
  

•   for _, ns := range namespaces {
  

I would argue that it's better to extract our streams from the build
configurations instead of iterating over all available namespaces for two
reasons: 1) since we build dep trees based on ICTs, buildConfigs is where we
want to look for streams, plus we might end up with more streams in our
results than needed (can we have an imageStream around w/o a respective
buildConfig?). 2) Searching through buildConfigs seems cheaper (no REST
calls) vs GET streams for every namespace. Anyway, if you follow what you
are doing right now, you should probably remove getRepos since it's won't
be used anymore.

---

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1693/files#r28945900

[smarterclayton]

Correct

----- Original Message -----

@@ -186,14 +189,24 @@ func RunBuildChain(f *clientcmd.Factory, cmd
*cobra.Command, args []string) erro
// Set the specified repo as the only one to output dependencies for
repos[repo] = tags
} else {

•   // Get all image repositories from build configurations
  

- repos = getRepos(buildConfigList)

•   // Make sure from now on that the --all flag is true
  

•   // since we are building dependency trees for every
  

•   // image repository available either in the current
  

•   // namespace or in every namespace
  

•   all = true
  

•   // all the tags of all the imagestreams across all the namespaces
  

•   for _, ns := range namespaces {
  

Image streams definitely can exist by themselves.

Sent from my iPhone

On Apr 23, 2015, at 4:46 AM, Michail Kargakis notifications@github.com
wrote:

In pkg/cmd/experimental/buildchain/buildchain.go:

@@ -186,14 +189,24 @@ func RunBuildChain(f *clientcmd.Factory, cmd
*cobra.Command, args []string) erro
// Set the specified repo as the only one to output dependencies for
repos[repo] = tags
} else {

• // Get all image repositories from build configurations
  

- repos = getRepos(buildConfigList)

• // Make sure from now on that the --all flag is true
  

• // since we are building dependency trees for every
  

• // image repository available either in the current
  

• // namespace or in every namespace
  

• all = true
  

• // all the tags of all the imagestreams across all the namespaces
  

• for _, ns := range namespaces {
  

  I would argue that it's better to extract our streams from the build
  configurations instead of iterating over all available namespaces for two
  reasons: 1) since we build dep trees based on ICTs, buildConfigs is where
  we want to look for streams, plus we might end up with more streams in our
  results than needed (can we have an imageStream around w/o a respective
  buildConfig?). 2) Searching through buildConfigs seems cheaper (no REST
  calls) vs GET streams for every namespace. Anyway, if you follow what you
  are doing right now, you should probably remove getRepos since it's won't
  be used anymore.

—
Reply to this email directly or view it on GitHub.

---

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1693/files#r28956035

[bparees]

You are already making a call per namespace to get the build configs. And you have to, thanks to our security model. (you cannot get all build configs across all visible namespaces.)

We can filter out streams with no dependencies if we want.

[0xmichalis]

We have to get the build configs in any case.

[bparees]

Right, my point is its going from o(n) to o(2n) not from o(1).  If we can't make calls per namespace, then we were already broken.  

From: Michail Kargakis notifications@github.com
Sent: Apr 23, 2015 5:53 PM
To: openshift/origin
Cc: Ben Parees
Subject: Re: [origin] add imagestream refs for custom and docker strategies (#1693)

@@ -186,14 +189,24 @@ func RunBuildChain(f *clientcmd.Factory, cmd *cobra.Command, args []string) erro
// Set the specified repo as the only one to output dependencies for
repos[repo] = tags
} else {

• // Get all image repositories from build configurations
  

- repos = getRepos(buildConfigList)

• // Make sure from now on that the --all flag is true
  

• // since we are building dependency trees for every
  

• // image repository available either in the current
  

• // namespace or in every namespace
  

• all = true
  

• // all the tags of all the imagestreams across all the namespaces
  

• for _, ns := range namespaces {
  

We have to get the build configs in any case.

---

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1693/files#r29007517

[bparees]

@smarterclayton @Kargakis i went ahead and reworked this to not make an imagestream call per project, but as noted earlier, we are still (and always have been) making a buildconfig call per namespace because the API will not return "all buildconfigs for all projects I can view" in a single call.

[0xmichalis]

Now that makes me happy:) One more nit, but as are the changes in build-chain LGTM

[bparees]

@smarterclayton added more conversion tests, reworked buildchain implementation... anything else?

[bparees]

@smarterclayton added second commit w/ the changes we discussed today... ptal and i'll squash if you're good with them.

[bparees]

@smarterclayton the overall design is that the internal representation of a From is always ImageStreamTag or DockerImage now, and From.Name is always of the form "name:tag". I don't remember if we agreed that was a safe assumption, but i'm pretty sure that's what @ncdc had told me (that :tag is not optional on an ImageStreamTag name)

I'm less certain about whether ImageStreamImage is being handled correctly everywhere, i'm not even sure if it can be done right until @ncdc 's v2 pull lands.

[smarterclayton]

Two small comments, then LGTM

[smarterclayton]

----- Original Message -----

@smarterclayton the overall design is that the internal representation of a
From is always ImageStreamTag or DockerImage now, and From.Name is always of
the form "name:tag". I don't remember if we agreed that was a safe
assumption, but i'm pretty sure that's what @ncdc had told me (that :tag is
not optional on an ImageStreamTag name)

Sounds correct mo e.

I'm less certain about whether ImageStreamImage is being handled correctly
everywhere, i'm not even sure if it can be done right until @ncdc 's v2 pull
lands.

It probably isn't - the warning just prevents us from forgetting about this forever.

---

Reply to this email directly or view it on GitHub:
#1693 (comment)

[bparees]

added the logging and changed the string compare. pushing merge and sending a note to the mailing lists.
[merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1719/) (Image: devenv-fedora_1393)

[openshift-bot]

Evaluated for origin up to 8f01437