Skip to content
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

Add an initial importer for Image Repositories #1303

Merged
merged 4 commits into from
Mar 17, 2015

Conversation

smarterclayton
Copy link
Contributor

Image Repositories that have "dockerImageRepository" set will have
images imported if "tags" is empty or if there are "tags" that have
an empty string value. Once complete, or if sufficient failures
occur, the annotation "openshift.io/image.dockerRepositoryCheck"
will be set to a timestamp (time completed) or a string message.

This won't work until we can pull by ID against the local registry.

@ncdc

@ncdc
Copy link
Contributor

ncdc commented Mar 13, 2015

Do you want this to support both v1 and v2 registries?

@smarterclayton
Copy link
Contributor Author

See my note about not being able to merge this util pull by id works.

On Mar 12, 2015, at 8:12 PM, Andy Goldstein [email protected] wrote:

Do you want this to support both v1 and v2 registries?


Reply to this email directly or view it on GitHub.

@smarterclayton
Copy link
Contributor Author

I might have it use (instead of image ids) the tags.

On Mar 12, 2015, at 8:12 PM, Andy Goldstein [email protected] wrote:

Do you want this to support both v1 and v2 registries?


Reply to this email directly or view it on GitHub.

@ncdc
Copy link
Contributor

ncdc commented Mar 13, 2015

I ask about supporting both versions of the registry as it means we'll need to modify things like getTags to try both v1 and v2 URL routes (https://github.com/openshift/origin/pull/1303/files#diff-9ebbd569a27134d93340002608ebe1a9R182)

@ncdc
Copy link
Contributor

ncdc commented Mar 13, 2015

And yes, for external v1 registries, I think tags might be the only real universal option.

@smarterclayton
Copy link
Contributor Author

I'll probably only support v1 for now. This is primarily for use existing v1 registry and get metadata.

On Mar 12, 2015, at 8:20 PM, Andy Goldstein [email protected] wrote:

I ask about supporting both versions of the registry as it means we'll need to modify things like getTags to try both v1 and v2 URL routes (https://github.com/openshift/origin/pull/1303/files#diff-9ebbd569a27134d93340002608ebe1a9R182)


Reply to this email directly or view it on GitHub.

@smarterclayton smarterclayton force-pushed the add_docker_controller branch from b357a7e to 8031e2a Compare March 13, 2015 00:32
@@ -154,6 +155,11 @@ func (c *MasterConfig) ImageChangeControllerClient() *osclient.Client {
return c.OSClient
}

// DeploymentClient returns the deployment client object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong method name

@smarterclayton smarterclayton force-pushed the add_docker_controller branch from 8031e2a to 2ebd614 Compare March 13, 2015 01:07
@smarterclayton
Copy link
Contributor Author

See the last commit - during imageRepositoryMapping, if the dockerImageReference has a different tag than mapping.Tag, the server can assume that the reference's tag is valid. Otherwise, it must use the tag provided. This will allow both scenarios to work now.

@smarterclayton
Copy link
Contributor Author

@ironcladlou please review the first commit. Standardized some things with retry handler (only one place needs to check errors).

@smarterclayton smarterclayton force-pushed the add_docker_controller branch 2 times, most recently from f363b36 to dc3e220 Compare March 13, 2015 03:26
@ironcladlou
Copy link
Contributor

@smarterclayton

@ironcladlou please review the first commit. Standardized some things with retry handler (only one place needs to check errors).

Made a few comments on the commit. Cool refactor!

@smarterclayton smarterclayton force-pushed the add_docker_controller branch from dc3e220 to 0f24309 Compare March 13, 2015 17:46
@ironcladlou
Copy link
Contributor

Controller changes LGTM!

@smarterclayton smarterclayton force-pushed the add_docker_controller branch 2 times, most recently from eeff9d1 to 828caa0 Compare March 15, 2015 00:56
@smarterclayton smarterclayton changed the title WIP - Add an initial importer for Image Repositories Add an initial importer for Image Repositories Mar 15, 2015
@smarterclayton
Copy link
Contributor Author

I added a significant number of changes to how tags and status events are stored. When a user sets a "Tag", they are making a reference to another object. There are only two types of references supported - a tag that already exists on the image repo (so you can have tag "foo" pointing to "latest"), or the image id reference described elsewhere. Image repository mappings are changed to post status updates directly to the image repository, vs making a spec change. As a result, the Spec.Tags and Status.Tags can differ - since @ncdc's earlier change made that a non issue (except in one case around builds), the change is less invasive. Consumers of the Status.Tags field should be aware that the "Image" field may be empty (the reference is purely tag oriented) or that the DockerImageReference may be empty (in which case the next older tag should be preferred).

@bparees please review the build controller changes - we can no longer assume that "Image" is set, which means that the value we set should either be Image (preferentially) or DockerImageReference (if Image is empty). That ensures that a change is triggered. This does not affect deployments because they rely on the template being changed to trigger a rollout.

@ncdc updateTagHistory is removed in favor of refactoring into two helper methods AddTagEventToImageRepository (which handles compression of related events) and TagValueToTagEvent (which handles splitting out the tag use case). I don't believe this should impact your changes - users will set tags to differentiate things (and can still set IDs), but the mapping part is now significantly different.

@smarterclayton smarterclayton force-pushed the add_docker_controller branch from 828caa0 to 471ccd1 Compare March 15, 2015 01:08
@smarterclayton
Copy link
Contributor Author

[test]

@openshift-bot
Copy link
Contributor

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

imageSubstitutions := make(map[string]string)
func (c *ImageChangeController) HandleImageRepo(repo *imageapi.ImageRepository) error {
glog.V(4).Infof("Build image change controller detected imagerepo change %s", repo.Status.DockerImageRepository)
subs := make(map[string]string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1

for _, trigger := range config.Triggers {
if trigger.Type != buildapi.ImageChangeBuildTriggerType {
continue
}
icTrigger := trigger.ImageChange
change := trigger.ImageChange
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1, it's not a change, it's a trigger. specifically an image change trigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it's the parameters to an image change trigger. It's the definition of what should change.

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

    for _, trigger := range config.Triggers {
        if trigger.Type != buildapi.ImageChangeBuildTriggerType {
            continue
        }
  •       icTrigger := trigger.ImageChange
    
  •       change := trigger.ImageChange
    

-1, it's not a change, it's a trigger. specifically an image change trigger.


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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the parameters would include the actual new imageid, this doesn't. this is the vessel, not the values.

@smarterclayton smarterclayton force-pushed the add_docker_controller branch from 6361372 to b9ad388 Compare March 16, 2015 14:47
next = latest.DockerImageReference
}
if len(last) == 0 || next != last {
subs[change.Image] = latest.DockerImageReference
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DockerImageReference is a fully qualified/tagged/immutable value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

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

            continue
        }

        // (must be different) to trigger a build
  •       if icTrigger.LastTriggeredImageID != latest.Image {
    
  •           imageSubstitutions[icTrigger.Image] = latest.DockerImageReference
    
  •           shouldTriggerBuild = true
    
  •           icTrigger.LastTriggeredImageID = latest.Image
    
  •       last := change.LastTriggeredImageID
    
  •       next := latest.Image
    
  •       if len(next) == 0 {
    
  •           // tags without images should still trigger builds (when going from a
    
    pure tag to an image
  •           // based tag, we should rebuild)
    
  •           next = latest.DockerImageReference
    
  •       }
    
  •       if len(last) == 0 || next != last {
    
  •           subs[change.Image] = latest.DockerImageReference
    

DockerImageReference is a fully qualified/tagged/immutable value?


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

@smarterclayton smarterclayton force-pushed the add_docker_controller branch from 593ceff to cd57d6f Compare March 17, 2015 16:57
@ncdc
Copy link
Contributor

ncdc commented Mar 17, 2015

This is a pretty big change and it's sometimes hard to keep all the priorities/conditionals straight, but I think I can wrap my head around it so...

LGTM

@smarterclayton
Copy link
Contributor Author

At worst, we break someone and fix it. :)

@ncdc
Copy link
Contributor

ncdc commented Mar 17, 2015

Exactly!

@smarterclayton
Copy link
Contributor Author

[merge]

@openshift-bot
Copy link
Contributor

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

smarterclayton and others added 4 commits March 17, 2015 16:15
Image Repositories that have "dockerImageRepository" set will have
images imported if "tags" is empty or if there are "tags" that have
an empty string value. Once complete, or if sufficient failures
occur, the annotation "openshift.io/image.dockerRepositoryCheck"
will be set to a timestamp (time completed) or a string message.
Short term solution to allow the registry to indicate an image is
pullable via another tag, while allowing initial repo population
to work based only on existing tags.
TagEvent is set via PUT /imageRepositories/foo/status in
ImageRepositoryMappings. This means that users and admins/infra
components can set spec tags and status tags independently.

User set tags are true tag mappings, with eventual support for
digests within the repository.  They still require the user have
access to the image in order to get the pull spec.
Also process all tags for an image id.
@smarterclayton smarterclayton force-pushed the add_docker_controller branch from cd57d6f to ebbf8a6 Compare March 17, 2015 20:17
@openshift-bot
Copy link
Contributor

Evaluated for origin up to ebbf8a6

openshift-bot pushed a commit that referenced this pull request Mar 17, 2015
@openshift-bot openshift-bot merged commit 83a7f0a into openshift:master Mar 17, 2015
@smarterclayton smarterclayton deleted the add_docker_controller branch May 18, 2015 02:16
jboyd01 pushed a commit to jboyd01/origin that referenced this pull request Feb 6, 2018
…service-catalog/' changes from d969acde90..b69b4a6c80

b69b4a6c80 origin build: modify hard coded path
527fac4d02 origin build: add origin tooling
545ffdb chart changes for v0.1.5 release (openshift#1709)
4d9be8f Use userInfo for Originating-Identity so extras is correct. (openshift#1702)
f358b99 Call destroy function on each storage interface (openshift#1705)
36b5de9 refactor binding reconciliation functions (openshift#1687)
5699360 Change binding_retrievable to bindingRetrievable
0d8bcfe thread through stopCh to DestroyFunc (openshift#1671)
1c45aef Migrate from glide to dep for dependency management (openshift#1670)
1cf0dd9 Add svcat to Makefile (openshift#1683)
45b1013 make verify validates that versioned APIs contain json tags for fields, addresses openshift#1303 (openshift#1480)
0ee8398 Build the integration test binary before running any tests (openshift#1666)
0fe0aa7 Update design.md (openshift#1674)
1280d24 controller requires permission to update secrets (openshift#1663)
129d98e Contribute svcat (openshift#1664)
ff9739b Update dependencies to kubernetes-1.9.1 (openshift#1633)
9c36019 chart changes for v0.1.4 release (openshift#1669)
93319f6 move apiserver generation to script and verify (openshift#1662)
385f0da refactor service instance provision/update/poll reconciliation (openshift#1648)
e015212 run each integration test individually (openshift#1661)
412e242 Tell people whether we're checking external hrefs (openshift#1659)
ae05361 retry failed unbind requests (openshift#1653)
7eae845 doc for setting up Service Catalog with Prometheus metrics (openshift#1654)
0720cf9 minor README copy edit (openshift#1656)
8bd347d run some integration subtests in parallel (openshift#1637)
b83800c Use $ and console to indicate multi-command blocks
789c4b2 Use dynamic reaction to fix data race (openshift#1650)
f1be763 only check external hrefs on master (openshift#1652)
65c6d20 Controller-manager crash loops if API server is not available on startup (openshift#1376) (openshift#1591)
9225c92 embedded etcd is the way of the future for our tests (openshift#1651)
605c952 Fix required fields in OpenAPI schema (openshift#1602)
899ca21 Revert "Switch to wget for integration apiserver checks (openshift#1384)" (openshift#1585)
2f496ee Update code-of-conduct.md (openshift#1635)
c1c69cf Build the e2e binary in CI (openshift#1647)
4e2dcef Wait for successful API discovery (openshift#1646)
5ae6d99 Bump copyright date in generated code (openshift#1645)
8be5b05 Serve OpenAPI spec only when --serve-openapi-spec switch is enabled (openshift#1612)
19fb30e silence go-restful logging output (openshift#1622)
fdbabf0 Add walkthrough link back (openshift#1620)
7c73e9a Add link to main k8s docs on service-catalog (openshift#1627)
f59adc9 Overhauling the design document (openshift#1619)
cd7b633 Updating the install documentation (openshift#1616)
f6e5441 fix compilation error from updated util.WaitForBindingCondition() (openshift#1629) (openshift#1631)
54e57af Provide OSB Client proxy to instrument with metrics (openshift#1615)
026b86f Disable test added in 1611 that contains data race (openshift#1626)
cb735a6 Add integration tests for ServiceInstances (openshift#1611)
67dbabb Cleaning up the docs README (openshift#1618)
6bddc07 remove email from docker login during Travis deploy (openshift#1614)
a604bc3 Use ConfigMaps for leader election (openshift#1599)
c6f193a Add controller integration tests for ServiceInstance create and update (openshift#1578)
26cf23b Rename OWNERS assignees: to approvers: (openshift#1508)
1163edc expose Prometheus metrics from Controller (openshift#677) (openshift#1608)
2cd6554 Clean up docs/ folder (openshift#1609)
1d7e96d Adding Service Binding Create Integration Tests (openshift#1580)
6a4c469 Make the maximum polling backoff configurable (openshift#1607)
31bbf55 Rename the imported package to avoid name conflict (openshift#1603)
3cdd556 Add validation for broker spec to SAR admission controller (openshift#1605)
a3408ce fix docker volume mount when building with docker under SELinux (openshift#1500) (openshift#1534)
307e747 Remove unneeded vendors of vendors (openshift#1596)
770fc74 Make ups-instance.yaml in walkthrough to demonstrate good practices (openshift#1592)
9112ba1 Add links to docs/README (openshift#1589)
8902648 Add additional service to ups-broker to fix e2e (openshift#1583)
0bcbc7d move instance update logic out of reconcileServiceInstanceDelete (openshift#1584)
7ef5a3e Do not send Parameters field when there are no parameters from sourced secret (openshift#1559)
4c51b25 Remove unneeded code that sets reason for provision/update call failure (openshift#1561)
b122cb9 fix bind injection failure not being persisted in API server (openshift#1546)
66421d5 Clear out current operation when starting reconciliation of a delete (openshift#1564)
8cca70a Send an empty object for Parameters when deleting all parameters of a ServiceInstance (openshift#1555)
270426c Add controllerTest type as a helper for running controller integration tests. (openshift#1577)
e26c2d7 Ignore .vscode folder in project root (openshift#1579)
REVERT: d969acde90 Add additional service to ups-broker to fix e2e (openshift#1583)
REVERT: 1bcd53b684 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: b69b4a6c8003f25d040e3087c7b1b16d1854a9e9
Miciah pushed a commit to Miciah/origin that referenced this pull request Jun 27, 2018
build: handle empty commits in bitbucket webhook payload
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants