diff --git a/cmd/release-controller-api/http.go b/cmd/release-controller-api/http.go index 066427086..87e9ffd51 100644 --- a/cmd/release-controller-api/http.go +++ b/cmd/release-controller-api/http.go @@ -121,7 +121,7 @@ func (c *Controller) findReleaseStreamTags(includeStableTags bool, tags ...strin needed[tag.Name] = &ReleaseStreamTag{ Release: r, Tag: tag, - Previous: findPreviousRelease(releaseTags[i+1:], r), + Previous: findPreviousRelease(releaseTags[i+1:], r, c.resolvePhase), PreviousRelease: r, Older: releaseTags[i+1:], Stable: stable, @@ -550,7 +550,7 @@ func (c *Controller) apiReleaseLatest(w http.ResponseWriter, req *http.Request) Name: latest.Name, PullSpec: resolveReleasePullSpec(r, latest.Name), DownloadURL: downloadURL, - Phase: latest.Annotations[releasecontroller.ReleaseAnnotationPhase], + Phase: c.resolvePhase(*latest), } switch req.URL.Query().Get("format") { @@ -627,7 +627,7 @@ func (c *Controller) apiReleaseTags(w http.ResponseWriter, req *http.Request) { var tags []releasecontroller.APITag for _, tag := range r.Tags { downloadURL, _ := c.urlForArtifacts(tag.Name) - phase := tag.Annotations[releasecontroller.ReleaseAnnotationPhase] + phase := c.resolvePhase(*tag) if len(filterPhase) > 0 && !slices.Contains(filterPhase, phase) { continue } @@ -802,7 +802,7 @@ func (c *Controller) apiReleaseInfo(w http.ResponseWriter, req *http.Request) { summary := releasecontroller.APIReleaseInfo{ Name: tagInfo.Tag, - Phase: tagInfo.Info.Tag.Annotations[releasecontroller.ReleaseAnnotationPhase], + Phase: c.resolvePhase(*tagInfo.Info.Tag), Results: verificationJobs, UpgradesTo: c.graph.UpgradesTo(tagInfo.Tag), UpgradesFrom: c.graph.UpgradesFrom(tagInfo.Tag), @@ -1412,9 +1412,12 @@ func (c *Controller) httpReleaseInfo(w http.ResponseWriter, req *http.Request) { linkifyURLs(override.Reason)) } - switch tagInfo.Info.Tag.Annotations[releasecontroller.ReleaseAnnotationPhase] { + switch c.resolvePhase(*tagInfo.Info.Tag) { case releasecontroller.ReleasePhaseFailed: - fmt.Fprintf(w, `

%s

`, template.HTMLEscapeString(tagInfo.Info.Tag.Annotations[releasecontroller.ReleaseAnnotationMessage])) + fmt.Fprintf(w, `
`) + if payload := c.GetReleasePayload(tagInfo.Tag); payload != nil && payload.Spec.PayloadOverride.Reason != "" { + fmt.Fprintf(w, `

%s

`, template.HTMLEscapeString(payload.Spec.PayloadOverride.Reason)) + } if log := tagInfo.Info.Tag.Annotations[releasecontroller.ReleaseAnnotationLog]; len(log) > 0 { fmt.Fprintf(w, `
%s
`, template.HTMLEscapeString(log)) } else { @@ -1657,19 +1660,21 @@ func (c *Controller) doesInconsistencyExist(tag string) bool { } func (c *Controller) tableLink(config *releasecontroller.ReleaseConfig, tag imagev1.TagReference) string { - if canLink(tag) { - if value, ok := tag.Annotations[releasecontroller.ReleaseAnnotationKeep]; ok { - return fmt.Sprintf(`%s *`, template.HTMLEscapeString(value), phaseAlert(tag), template.HTMLEscapeString(config.Name), template.HTMLEscapeString(tag.Name), template.HTMLEscapeString(tag.Name)) + phase := c.resolvePhase(tag) + alert := phaseAlert(phase) + if canLink(phase) { + if _, ok := tag.Annotations[releasecontroller.ReleaseAnnotationKeep]; ok { + return fmt.Sprintf(`%s *`, template.HTMLEscapeString("keep"), alert, template.HTMLEscapeString(config.Name), template.HTMLEscapeString(tag.Name), template.HTMLEscapeString(tag.Name)) } if strings.Contains(tag.Name, "nightly") && c.doesInconsistencyExist(tag.Name) { - return fmt.Sprintf(`%s `, phaseAlert(tag), template.HTMLEscapeString(config.Name), template.HTMLEscapeString(tag.Name), template.HTMLEscapeString(tag.Name), template.HTMLEscapeString(config.Name), template.HTMLEscapeString(tag.Name)) + return fmt.Sprintf(`%s `, alert, template.HTMLEscapeString(config.Name), template.HTMLEscapeString(tag.Name), template.HTMLEscapeString(tag.Name), template.HTMLEscapeString(config.Name), template.HTMLEscapeString(tag.Name)) } else if config.As == releasecontroller.ReleaseConfigModeStable { - return fmt.Sprintf(`%s`, phaseAlert(tag), template.HTMLEscapeString(config.Name), template.HTMLEscapeString(tag.Name), template.HTMLEscapeString(tag.Name)) + return fmt.Sprintf(`%s`, alert, template.HTMLEscapeString(config.Name), template.HTMLEscapeString(tag.Name), template.HTMLEscapeString(tag.Name)) } else { - return fmt.Sprintf(`%s`, phaseAlert(tag), template.HTMLEscapeString(config.Name), template.HTMLEscapeString(tag.Name), template.HTMLEscapeString(tag.Name)) + return fmt.Sprintf(`%s`, alert, template.HTMLEscapeString(config.Name), template.HTMLEscapeString(tag.Name), template.HTMLEscapeString(tag.Name)) } } - return fmt.Sprintf(`%s`, phaseAlert(tag), template.HTMLEscapeString(tag.Name)) + return fmt.Sprintf(`%s`, alert, template.HTMLEscapeString(tag.Name)) } func (c *Controller) httpReleases(w http.ResponseWriter, req *http.Request) { @@ -1756,7 +1761,6 @@ func (c *Controller) httpReleases(w http.ResponseWriter, req *http.Request) { "versionGrouping": versionGrouping, "streamNames": streamNames, "phaseCell": c.phaseCell, - "phaseAlert": phaseAlert, "alerts": renderAlerts, "links": c.links, "releaseJoin": releaseJoin, @@ -1996,7 +2000,6 @@ func (c *Controller) httpReleaseStreamTable(w http.ResponseWriter, req *http.Req "versionGrouping": versionGrouping, "streamNames": streamNames, "phaseCell": c.phaseCell, - "phaseAlert": phaseAlert, "alerts": renderAlerts, "links": c.links, "releaseJoin": releaseJoin, @@ -2144,7 +2147,6 @@ func (c *Controller) httpDashboardOverview(w http.ResponseWriter, req *http.Requ }, "tableLink": c.tableLink, "phaseCell": c.phaseCell, - "phaseAlert": phaseAlert, "inc": func(i int) int { return i + 1 }, "upgradeJobs": upgradeJobs, "releaseJoin": releaseJoin, @@ -2186,7 +2188,11 @@ func (c *Controller) httpDashboardOverview(w http.ResponseWriter, req *http.Requ delays = append(delays, fmt.Sprintf("no more than %d pending", r.Config.MaxUnreadyReleases)) } } - if isReleaseFailing(s.Tags, r.Config.MaxUnreadyReleases) { + phases := make([]string, len(s.Tags)) + for i, tag := range s.Tags { + phases[i] = c.resolvePhase(*tag) + } + if isReleaseFailing(phases, r.Config.MaxUnreadyReleases) { s.Failing = true } @@ -2223,10 +2229,13 @@ func generateStreamMessage(r *ReleaseStream) string { return fmt.Sprintf("%s%s", prefix, message) } -func isReleaseFailing(tags []*imagev1.TagReference, maxUnready int) bool { +func isReleaseFailing(phases []string, maxUnready int) bool { + if maxUnready == 0 { + return false + } unreadyCount := 0 - for i := 0; unreadyCount < maxUnready && i < len(tags); i++ { - switch tags[i].Annotations[releasecontroller.ReleaseAnnotationPhase] { + for i := 0; unreadyCount < maxUnready && i < len(phases); i++ { + switch phases[i] { case releasecontroller.ReleasePhaseReady: continue case releasecontroller.ReleasePhaseAccepted: @@ -2422,10 +2431,8 @@ func (c *Controller) filteredStreams(phase string) ([]byte, error) { if phase == "" { tags = append(tags, tag.Name) } else { - if annotation, ok := tag.Annotations[releasecontroller.ReleaseAnnotationPhase]; ok { - if annotation == phase { - tags = append(tags, tag.Name) - } + if c.resolvePhase(*tag) == phase { + tags = append(tags, tag.Name) } } } diff --git a/cmd/release-controller-api/http_candidate.go b/cmd/release-controller-api/http_candidate.go index 217c213ee..96840e255 100644 --- a/cmd/release-controller-api/http_candidate.go +++ b/cmd/release-controller-api/http_candidate.go @@ -237,7 +237,7 @@ func (c *Controller) findReleaseCandidates(upgradeSuccessPercent float64, releas candidates := make([]*releasecontroller.ReleaseCandidate, 0) releaseTags := releasecontroller.SortedReleaseTags(releaseStreamTagMap[stream].Release) for _, tag := range releaseTags { - if tag.Annotations != nil && tag.Annotations[releasecontroller.ReleaseAnnotationPhase] == releasecontroller.ReleasePhaseAccepted && + if c.resolvePhase(*tag) == releasecontroller.ReleasePhaseAccepted && tag.Annotations[releasecontroller.ReleaseAnnotationCreationTimestamp] != "" { t, _ := time.Parse(time.RFC3339, tag.Annotations[releasecontroller.ReleaseAnnotationCreationTimestamp]) ts := t.Unix() diff --git a/cmd/release-controller-api/http_helper.go b/cmd/release-controller-api/http_helper.go index 4ae2644a0..fb0d8f917 100644 --- a/cmd/release-controller-api/http_helper.go +++ b/cmd/release-controller-api/http_helper.go @@ -130,17 +130,24 @@ func resolveReleasePullSpec(release *releasecontroller.Release, tagName string) return releasecontroller.FindPublicImagePullSpec(release.Target, tagName) } +func (c *Controller) resolvePhase(tag imagev1.TagReference) string { + payload := c.GetReleasePayload(tag.Name) + if payload == nil { + klog.Errorf("no ReleasePayload found for tag %q, phase is unknown", tag.Name) + return "Unknown" + } + return releasecontroller.GetReleasePhase(payload) +} + func (c *Controller) phaseCell(tag imagev1.TagReference) string { - phase := tag.Annotations[releasecontroller.ReleaseAnnotationPhase] - alert := phaseAlert(tag) + phase := c.resolvePhase(tag) + alert := phaseAlert(phase) var title string var overridden bool if payload := c.GetReleasePayload(tag.Name); payload != nil && payload.Spec.PayloadOverride.Reason != "" { title = payload.Spec.PayloadOverride.Reason overridden = true - } else if phase == releasecontroller.ReleasePhaseRejected { - title = tag.Annotations[releasecontroller.ReleaseAnnotationMessage] } display := template.HTMLEscapeString(phase) @@ -158,8 +165,7 @@ func (c *Controller) phaseCell(tag imagev1.TagReference) string { return fmt.Sprintf("%s", alert, display) } -func phaseAlert(tag imagev1.TagReference) string { - phase := tag.Annotations[releasecontroller.ReleaseAnnotationPhase] +func phaseAlert(phase string) string { switch phase { case releasecontroller.ReleasePhasePending: return "" @@ -295,13 +301,8 @@ func upgradeJobs(upgrades *ReleaseUpgrades, index int, tagCreationTimestampStrin return buf2.String() } -func canLink(tag imagev1.TagReference) bool { - switch tag.Annotations[releasecontroller.ReleaseAnnotationPhase] { - case releasecontroller.ReleasePhasePending: - return false - default: - return true - } +func canLink(phase string) bool { + return phase != releasecontroller.ReleasePhasePending } func (c *Controller) GetReleasePayload(name string) *v1alpha1.ReleasePayload { @@ -409,7 +410,8 @@ func (c *Controller) links(tag imagev1.TagReference, release *releasecontroller. buf.WriteString("") continue } - final := tag.Annotations[releasecontroller.ReleaseAnnotationPhase] == releasecontroller.ReleasePhaseRejected || tag.Annotations[releasecontroller.ReleaseAnnotationPhase] == releasecontroller.ReleasePhaseAccepted + phase := c.resolvePhase(tag) + final := phase == releasecontroller.ReleasePhaseRejected || phase == releasecontroller.ReleasePhaseAccepted if !verificationJobs[key].Disabled && !final { buf.WriteString(" ") buf.WriteString(template.HTMLEscapeString(key)) @@ -487,7 +489,8 @@ func (c *Controller) renderVerifyLinks(w io.Writer, tag imagev1.TagReference, re return } buf := &bytes.Buffer{} - final := tag.Annotations[releasecontroller.ReleaseAnnotationPhase] == releasecontroller.ReleasePhaseRejected || tag.Annotations[releasecontroller.ReleaseAnnotationPhase] == releasecontroller.ReleasePhaseAccepted + phase := c.resolvePhase(tag) + final := phase == releasecontroller.ReleasePhaseRejected || phase == releasecontroller.ReleasePhaseAccepted if len(verificationJobs.BlockingJobs) > 0 { buf.WriteString("
  • Blocking jobs
      ") buf.WriteString(c.renderVerificationJobsList(verificationJobs.BlockingJobs, release, tag, final)) @@ -667,7 +670,7 @@ func hasPublishTag(config *releasecontroller.ReleaseConfig) (string, bool) { return "", false } -func findPreviousRelease(older []*imagev1.TagReference, release *releasecontroller.Release) *imagev1.TagReference { +func findPreviousRelease(older []*imagev1.TagReference, release *releasecontroller.Release, phaseOf func(imagev1.TagReference) string) *imagev1.TagReference { if len(older) == 0 { return nil } @@ -682,7 +685,7 @@ func findPreviousRelease(older []*imagev1.TagReference, release *releasecontroll } } for _, old := range older { - if old.Annotations[releasecontroller.ReleaseAnnotationPhase] == releasecontroller.ReleasePhaseAccepted { + if phaseOf(*old) == releasecontroller.ReleasePhaseAccepted { return old } } diff --git a/hack/release-tool.py b/hack/release-tool.py index 56cdf97a9..5ed6dbcdb 100755 --- a/hack/release-tool.py +++ b/hack/release-tool.py @@ -45,55 +45,11 @@ def validate_server_connection(ctx): username = oc.whoami() version = oc.get_server_version() logger.debug(f'Connected to APIServer running version: {version}, as: {username}') - except (ValueError, OpenShiftPythonException, Exception) as e: + except (OpenShiftPythonException, Exception) as e: logger.error(f"Unable to verify cluster connection using context: \"{ctx['context']}\"") raise e -def create_imagestreamtag_patch(action, custom_message, custom_reason): - data = { - 'image': { - 'metadata': { - 'annotations': {} - } - }, - 'metadata': { - 'annotations': {} - }, - 'tag': { - 'annotations': {} - } - } - - if action == 'accept': - phase = 'Accepted' - elif action == 'reject': - phase = 'Rejected' - else: - raise ValueError(f'Unsupported action specified: {action}') - - message = f'Manually {action}ed per TRT' - if custom_message is not None: - message = custom_message - - annotations = { - 'phase': phase, - 'message': message - } - - if custom_reason is not None: - annotations['reason'] = custom_reason - - for key, value in annotations.items(): - if value is not None: - annotation = 'release.openshift.io/' + key - data['image']['metadata']['annotations'][annotation] = value - data['metadata']['annotations'][annotation] = value - data['tag']['annotations'][annotation] = value - - return data - - def write_backup_file(path, name, release, data): ts = int(round(time.time() * 1000)) backup_filename = f'{path}/{name}_{release}-{ts}.json' @@ -105,34 +61,6 @@ def write_backup_file(path, name, release, data): return backup_filename -def patch_imagestreamtag(ctx, namespace, imagestream, action, release, custom_message, custom_reason, execute, output_path): - patch = create_imagestreamtag_patch(action, custom_message, custom_reason) - logger.debug(f'Generated oc patch:\n{json.dumps(patch, indent=4)}') - - with oc.options(ctx), oc.tracking(), oc.timeout(15): - try: - with oc.project(namespace): - tag = oc.selector(f'imagestreamtag/{imagestream}:{release}').object(ignore_not_found=True) - if not tag: - raise ValueError(f'Unable to locate imagestreamtag: {namespace}/{imagestream}:{release}') - - logger.info(f'{action.capitalize()}ing imagestreamtag: {namespace}/{imagestream}:{release}') - if execute: - backup_file = write_backup_file(output_path, imagestream, release, tag.model._primitive()) - - tag.patch(patch) - - logger.info(f'Release {release} updated successfully') - logger.info(f'Backup written to: {backup_file}') - else: - logger.info(f'[dry-run] Patching release {release} with patch:\n{json.dumps(patch, indent=4)}') - logger.warning('You must specify "--execute" to permanently apply these changes') - - except (ValueError, OpenShiftPythonException, Exception) as e: - logger.error(f'Unable to update release: "{release}"') - raise e - - def create_releasepayload_patch(action, custom_reason): if action == 'accept': override = 'Accepted' @@ -170,13 +98,13 @@ def resolve_imagestream_from_releasepayload(ctx, namespace, release): return None -def patch_releaespayload(ctx, namespace, action, release, custom_reason, execute, output_path): +def patch_releaespayload(ctx, options, namespace, action, release, custom_reason, execute, output_path): patch = create_releasepayload_patch(action, custom_reason) logger.debug(f'Generated oc patch:\n{json.dumps(patch, indent=4)}') with oc.options(ctx), oc.tracking(), oc.timeout(15): try: - with oc.project(namespace): + with oc.project(namespace), oc.options(options): payload = oc.selector(f'releasepayload/{release}').object(ignore_not_found=True) if not payload: logger.error(f'Unable to locate releasepayload: {namespace}/{release}') @@ -194,7 +122,7 @@ def patch_releaespayload(ctx, namespace, action, release, custom_reason, execute logger.info(f'[dry-run] Patching releasepayload {release} with patch:\n{json.dumps(patch, indent=4)}') logger.warning('You must specify "--execute" to permanently apply these changes') - except (ValueError, OpenShiftPythonException, Exception) as e: + except (OpenShiftPythonException, Exception) as e: logger.error(f'Unable to update releasepayload: "{release}"') raise e @@ -236,8 +164,8 @@ def delete_imagestreamtag(ctx, namespace, imagestream, tag, confirm, output_path logger.info(f'Deletion of imagestreamtag: "{namespace}/{imagestreamtag}" skipped.') else: logger.info(f'Imagestreamtag: "{namespace}/{imagestreamtag}" does not exist.') - except (ValueError, OpenShiftPythonException, Exception) as e: - logger.error(f'Unable to delete imagestreamtag: {e}') + except (OpenShiftPythonException, Exception) as e: + logger.exception(f'Unable to delete imagestreamtag: {e}') raise e @@ -315,8 +243,8 @@ def process_imagestream(ctx, namespace, imagestream, prefixes): tags_to_archive.append(data) else: logger.info(f'Imagestream: "{namespace}/{imagestream}" does not exist.') - except (ValueError, OpenShiftPythonException, Exception) as e: - logger.error(f'Unable to process imagestream: {e}') + except (OpenShiftPythonException, Exception) as e: + logger.exception(f'Unable to process imagestream: {e}') raise e return items @@ -389,7 +317,7 @@ def create_archive_imagestream(ctx, namespace, name, payload, execute): else: logger.info(f'[dry-run] Creating archive imagestream: {namespace}/{name}') logger.warning('You must specify "--execute" to permanently apply these changes') - except (ValueError, OpenShiftPythonException, Exception) as e: + except (OpenShiftPythonException, Exception) as e: logger.error(f'Unable to create archive imagestream: "{namespace}/{name}"') raise e @@ -420,7 +348,7 @@ def patch_archive_imagestream(ctx, namespace, name, status_tags, execute, output else: logger.info(f'[dry-run] Patching archive imagestream {name} with patch:\n{json.dumps(patch, indent=4)}') logger.warning('You must specify "--execute" to permanently apply these changes') - except (ValueError, OpenShiftPythonException, Exception) as e: + except (OpenShiftPythonException, Exception) as e: logger.error(f'Unable to update archive imagestream: "{name}"') raise e @@ -615,7 +543,7 @@ def bypass(ctx, nightly: str, trigger: str, execute): else: logger.info(f'[dry-run] oc tag with {tag_cmd_args} then {untag_cmd_args}') - except (ValueError, OpenShiftPythonException, Exception): + except (OpenShiftPythonException, Exception): logger.error(f'Unable to perform bypass of {nightly_components.major_minor} nightly {nightly}') raise @@ -650,19 +578,19 @@ def keep(ctx, namespace, imagestream, release, delete, execute): if tag.get_annotation('release.openshift.io/keep') is not None: logger.info(f' - {namespace}/{tag.name()}') - except (ValueError, OpenShiftPythonException, Exception) as e: + except (OpenShiftPythonException, Exception) as e: logger.error(f'Unable to process imagestreamtag: "{namespace}/{imagestream}:{release}"') raise e -def approval(ctx, namespace, imagestream, release, team, accept, reject, delete, check, execute): +def approval(ctx, options, namespace, release, team, accept, reject, delete, check, execute): team_label = team.lower() with oc.options(ctx), oc.tracking(), oc.timeout(5 * 60): try: - with oc.project(namespace): + with oc.options(options), oc.project(namespace): payload = oc.selector(f'releasepayload/{release}').object(ignore_not_found=True) if not payload: - logger.error(f'Unable to locate imagestreamtag: releasepayload/{release}') + logger.error(f'Unable to locate releasepayload/{release}') return if execute: if accept: @@ -689,8 +617,8 @@ def approval(ctx, namespace, imagestream, release, team, accept, reject, delete, else: logger.info(f'releasepayload/{release} marked as {state} by {team}') - except (ValueError, OpenShiftPythonException, Exception) as e: - logger.error(f'Unable to process imagestreamtag: "{namespace}/{imagestream}:{release}"') + except (OpenShiftPythonException, Exception) as e: + logger.error(f'Unable to process releasepayload: "{namespace}/releasepayload/{release}"') raise e @@ -726,7 +654,7 @@ def poke(ctx, namespace, art_imagestream_name, execute): logger.info('Triggered a release-controller update cycle.') else: logger.info(f'[dry-run] oc tag with {tag_cmd_args} then {untag_cmd_args}') - except (ValueError, OpenShiftPythonException, Exception): + except (OpenShiftPythonException, Exception): logger.error(f'Unable to poke imagestream: {namespace}/{art_imagestream_name}') raise @@ -743,7 +671,7 @@ def annotate_imagestream(ctx, namespace, name, annotations, execute): else: logger.info(f'[dry-run] Annotating imagestream {namespace}/{name} with: {annotations}') - except (ValueError, OpenShiftPythonException, Exception): + except (OpenShiftPythonException, Exception): logger.error(f'Unable to annotate imagestream: {namespace}/{name}') raise @@ -757,6 +685,7 @@ def annotate_imagestream(ctx, namespace, name, annotations, execute): config_group = parser.add_argument_group('Configuration Options') config_group.add_argument('-v', '--verbose', help='Enable verbose output', action='store_true') + config_group.add_argument('--admin', help='Run commands as "system:admin"', action='store_true') ocp_group = parser.add_argument_group('Openshift Configuration Options') ocp_group.add_argument('-c', '--context', help='The OC context to use (default is "app.ci")', default='app.ci') @@ -838,6 +767,11 @@ def annotate_imagestream(ctx, namespace, name, annotations, execute): validate_server_connection(context) + # Allow for a system:admin override if necessary + options = {} + if args['admin']: + options['as'] = 'system:admin' + # Configure the output location if args['output'] is None: output_dir = tempfile.mkdtemp(prefix=f'release-tool_{args["action"]}-') @@ -885,10 +819,7 @@ def annotate_imagestream(ctx, namespace, name, annotations, execute): # Execute action if args['action'] in ['accept', 'reject']: - # TODO: Remove once ReleasePayloads are fully implemented... - patch_imagestreamtag(context, release_namespace, release_image_stream, args['action'], args['release'], args['message'], args['reason'], args['execute'], output_dir) - - patch_releaespayload(context, release_namespace, args['action'], args['release'], args['reason'], args['execute'], output_dir) + patch_releaespayload(context, options, release_namespace, args['action'], args['release'], args['reason'], args['execute'], output_dir) elif args['action'] == 'prune': prune_release_tags(context, release_namespace, release_image_stream, args['releases'], args['execute'], args['yes'], output_dir) elif args['action'] == 'archive': @@ -929,7 +860,7 @@ def annotate_imagestream(ctx, namespace, name, annotations, execute): exit(1) team = args['check_team'] check = True - approval(context, release_namespace, release_image_stream, args['release'][0], team, accept, reject, delete, check, args['execute']) + approval(context, options, release_namespace, args['release'][0], team, accept, reject, delete, check, args['execute']) elif args['action'] == 'lock': # Generate the nightly imagestream information based on version nightly_namespace, nightly_imagestream = generate_resource_values(args['name'], f'{args["version"]}-art-latest', args['architecture'], args['private']) diff --git a/pkg/cmd/release-payload-controller/payload_accepted_controller_test.go b/pkg/cmd/release-payload-controller/payload_accepted_controller_test.go index f0247179e..b0436ffa8 100644 --- a/pkg/cmd/release-payload-controller/payload_accepted_controller_test.go +++ b/pkg/cmd/release-payload-controller/payload_accepted_controller_test.go @@ -442,6 +442,23 @@ func TestComputeReleasePayloadAcceptedCondition(t *testing.T) { Reason: ReleasePayloadAcceptedReason, }, }, + { + name: "BlockingJobWithEmptyAggregateState", + payload: &v1alpha1.ReleasePayload{ + Status: v1alpha1.ReleasePayloadStatus{ + BlockingJobResults: []v1alpha1.JobStatus{ + {AggregateState: v1alpha1.JobStateSuccess}, + {AggregateState: ""}, + {AggregateState: v1alpha1.JobStateSuccess}, + }, + }, + }, + expected: metav1.Condition{ + Type: v1alpha1.ConditionPayloadAccepted, + Status: metav1.ConditionUnknown, + Reason: ReleasePayloadAcceptedReason, + }, + }, { name: "NoBlockingJobs", payload: &v1alpha1.ReleasePayload{ diff --git a/pkg/cmd/release-payload-controller/payload_rejected_controller_test.go b/pkg/cmd/release-payload-controller/payload_rejected_controller_test.go index 69804e5e3..2dc713c29 100644 --- a/pkg/cmd/release-payload-controller/payload_rejected_controller_test.go +++ b/pkg/cmd/release-payload-controller/payload_rejected_controller_test.go @@ -395,6 +395,54 @@ func TestPayloadRejectedSync(t *testing.T) { }, }, }, + { + name: "ReleasePayloadWithFailedJobAndEmptyAggregateState", + input: &v1alpha1.ReleasePayload{ + ObjectMeta: metav1.ObjectMeta{ + Name: "4.11.0-0.nightly-2022-02-09-091559", + Namespace: "ocp", + }, + Status: v1alpha1.ReleasePayloadStatus{ + BlockingJobResults: []v1alpha1.JobStatus{ + { + AggregateState: v1alpha1.JobStateSuccess, + }, + { + AggregateState: v1alpha1.JobStateFailure, + }, + { + AggregateState: "", + }, + }, + }, + }, + expected: &v1alpha1.ReleasePayload{ + ObjectMeta: metav1.ObjectMeta{ + Name: "4.11.0-0.nightly-2022-02-09-091559", + Namespace: "ocp", + }, + Status: v1alpha1.ReleasePayloadStatus{ + BlockingJobResults: []v1alpha1.JobStatus{ + { + AggregateState: v1alpha1.JobStateSuccess, + }, + { + AggregateState: v1alpha1.JobStateFailure, + }, + { + AggregateState: "", + }, + }, + Conditions: []metav1.Condition{ + { + Type: v1alpha1.ConditionPayloadRejected, + Status: metav1.ConditionUnknown, + Reason: ReleasePayloadRejectedReason, + }, + }, + }, + }, + }, { name: "ReleaseCreationJobFailed", input: &v1alpha1.ReleasePayload{ diff --git a/pkg/release-controller/release_payload.go b/pkg/release-controller/release_payload.go index 829f8078b..151fc3cdc 100644 --- a/pkg/release-controller/release_payload.go +++ b/pkg/release-controller/release_payload.go @@ -35,6 +35,7 @@ func ApprovedReleases(lister v1alpha2.ReleasePayloadLister) ([]*v1alpha1.Release } func GetReleasePhase(payload *v1alpha1.ReleasePayload) string { + created := false for _, condition := range payload.Status.Conditions { switch condition.Type { case v1alpha1.ConditionPayloadAccepted: @@ -49,7 +50,14 @@ func GetReleasePhase(payload *v1alpha1.ReleasePayload) string { if condition.Status == v1.ConditionTrue { return ReleasePhaseFailed } + case v1alpha1.ConditionPayloadCreated: + if condition.Status == v1.ConditionTrue { + created = true + } } } - return "" + if created { + return ReleasePhaseReady + } + return ReleasePhasePending } diff --git a/pkg/release-controller/release_payload_test.go b/pkg/release-controller/release_payload_test.go new file mode 100644 index 000000000..9585471e7 --- /dev/null +++ b/pkg/release-controller/release_payload_test.go @@ -0,0 +1,93 @@ +package releasecontroller + +import ( + "testing" + + "github.com/openshift/release-controller/pkg/apis/release/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestGetReleasePhase(t *testing.T) { + tests := []struct { + name string + conditions []metav1.Condition + expected string + }{ + { + name: "no conditions returns Pending", + conditions: nil, + expected: ReleasePhasePending, + }, + { + name: "PayloadAccepted true returns Accepted", + conditions: []metav1.Condition{ + {Type: v1alpha1.ConditionPayloadCreated, Status: metav1.ConditionTrue}, + {Type: v1alpha1.ConditionPayloadAccepted, Status: metav1.ConditionTrue}, + }, + expected: ReleasePhaseAccepted, + }, + { + name: "PayloadRejected true returns Rejected", + conditions: []metav1.Condition{ + {Type: v1alpha1.ConditionPayloadCreated, Status: metav1.ConditionTrue}, + {Type: v1alpha1.ConditionPayloadRejected, Status: metav1.ConditionTrue}, + }, + expected: ReleasePhaseRejected, + }, + { + name: "PayloadFailed true returns Failed", + conditions: []metav1.Condition{ + {Type: v1alpha1.ConditionPayloadFailed, Status: metav1.ConditionTrue}, + }, + expected: ReleasePhaseFailed, + }, + { + name: "PayloadCreated true with no terminal returns Ready", + conditions: []metav1.Condition{ + {Type: v1alpha1.ConditionPayloadCreated, Status: metav1.ConditionTrue}, + {Type: v1alpha1.ConditionPayloadAccepted, Status: metav1.ConditionUnknown}, + {Type: v1alpha1.ConditionPayloadRejected, Status: metav1.ConditionUnknown}, + }, + expected: ReleasePhaseReady, + }, + { + name: "PayloadCreated false returns Pending", + conditions: []metav1.Condition{ + {Type: v1alpha1.ConditionPayloadCreated, Status: metav1.ConditionFalse}, + }, + expected: ReleasePhasePending, + }, + { + name: "terminal takes priority over created", + conditions: []metav1.Condition{ + {Type: v1alpha1.ConditionPayloadCreated, Status: metav1.ConditionTrue}, + {Type: v1alpha1.ConditionPayloadAccepted, Status: metav1.ConditionTrue}, + {Type: v1alpha1.ConditionPayloadRejected, Status: metav1.ConditionFalse}, + }, + expected: ReleasePhaseAccepted, + }, + { + name: "all conditions unknown returns Pending", + conditions: []metav1.Condition{ + {Type: v1alpha1.ConditionPayloadCreated, Status: metav1.ConditionUnknown}, + {Type: v1alpha1.ConditionPayloadAccepted, Status: metav1.ConditionUnknown}, + {Type: v1alpha1.ConditionPayloadRejected, Status: metav1.ConditionUnknown}, + }, + expected: ReleasePhasePending, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + payload := &v1alpha1.ReleasePayload{ + Status: v1alpha1.ReleasePayloadStatus{ + Conditions: tt.conditions, + }, + } + got := GetReleasePhase(payload) + if got != tt.expected { + t.Errorf("GetReleasePhase() = %q, want %q", got, tt.expected) + } + }) + } +} diff --git a/pkg/releasepayload/jobstatus/helpers.go b/pkg/releasepayload/jobstatus/helpers.go index 68640311d..444845bd7 100644 --- a/pkg/releasepayload/jobstatus/helpers.go +++ b/pkg/releasepayload/jobstatus/helpers.go @@ -62,6 +62,7 @@ func FindJobStatus(results []v1alpha1.JobStatus, ciConfigurationName, ciConfigur func ComputeJobState(jobs []v1alpha1.JobStatus) v1alpha1.JobState { totalJobs := len(jobs) var pendingJobs, successfulJobs, failedJobs []v1alpha1.JobStatus + undetermined := 0 for _, job := range jobs { switch job.AggregateState { @@ -71,19 +72,20 @@ func ComputeJobState(jobs []v1alpha1.JobStatus) v1alpha1.JobState { successfulJobs = append(successfulJobs, job) case v1alpha1.JobStateFailure: failedJobs = append(failedJobs, job) + default: + undetermined++ } } switch { - // Any failed jobs mean the release cannot be Accepted + case len(pendingJobs) > 0: + return v1alpha1.JobStatePending + case undetermined > 0: + return v1alpha1.JobStateUnknown case len(failedJobs) > 0: return v1alpha1.JobStateFailure - // If everything is successful, then we can Accept the payload case len(successfulJobs) == totalJobs && totalJobs > 0: return v1alpha1.JobStateSuccess - // If there are any pending jobs, then we're still working on validating the release - case len(pendingJobs) > 0: - return v1alpha1.JobStatePending default: return v1alpha1.JobStateUnknown } diff --git a/pkg/releasepayload/jobstatus/helpers_test.go b/pkg/releasepayload/jobstatus/helpers_test.go index 97c923688..733d527ef 100644 --- a/pkg/releasepayload/jobstatus/helpers_test.go +++ b/pkg/releasepayload/jobstatus/helpers_test.go @@ -73,6 +73,21 @@ func TestComputeJobState(t *testing.T) { }, expected: v1alpha1.JobStateFailure, }, + { + name: "FailedAndPendingJobs", + input: []v1alpha1.JobStatus{ + { + AggregateState: v1alpha1.JobStateSuccess, + }, + { + AggregateState: v1alpha1.JobStateFailure, + }, + { + AggregateState: v1alpha1.JobStatePending, + }, + }, + expected: v1alpha1.JobStatePending, + }, { name: "Garbage", input: []v1alpha1.JobStatus{ @@ -88,6 +103,48 @@ func TestComputeJobState(t *testing.T) { }, expected: v1alpha1.JobStateUnknown, }, + { + name: "FailedJobWithEmptyAggregateState", + input: []v1alpha1.JobStatus{ + { + AggregateState: v1alpha1.JobStateFailure, + }, + { + AggregateState: "", + }, + { + AggregateState: v1alpha1.JobStateSuccess, + }, + }, + expected: v1alpha1.JobStateUnknown, + }, + { + name: "SuccessWithEmptyAggregateState", + input: []v1alpha1.JobStatus{ + { + AggregateState: v1alpha1.JobStateSuccess, + }, + { + AggregateState: "", + }, + }, + expected: v1alpha1.JobStateUnknown, + }, + { + name: "PendingTakesPriorityOverUndetermined", + input: []v1alpha1.JobStatus{ + { + AggregateState: v1alpha1.JobStatePending, + }, + { + AggregateState: "", + }, + { + AggregateState: v1alpha1.JobStateFailure, + }, + }, + expected: v1alpha1.JobStatePending, + }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) {