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

fix(experiments): fire rollout event on experiment step #4124

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
dist/
*.iml
# delve debug binaries
__debug_bin
__debug_bin*
cmd/**/debug
debug.test
coverage.out
Expand All @@ -19,4 +19,4 @@ server/static/*
!server/static/.gitkeep
coverage-output-e2e/
coverage-output-unit/
junit-unit-test.xml
junit-unit-test.xml
3 changes: 1 addition & 2 deletions experiments/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ type Controller struct {
resyncPeriod time.Duration
}

// ControllerConfig describes the data required to instantiate a new analysis controller
// ControllerConfig describes the data required to instantiate a new experiments controller
type ControllerConfig struct {
KubeClientSet kubernetes.Interface
ArgoProjClientset clientset.Interface
Expand All @@ -100,7 +100,6 @@ type ControllerConfig struct {

// NewController returns a new experiment controller
func NewController(cfg ControllerConfig) *Controller {

replicaSetControl := controller.RealRSControl{
KubeClient: cfg.KubeClientSet,
Recorder: cfg.Recorder.K8sRecorder(),
Expand Down
16 changes: 13 additions & 3 deletions experiments/experiment.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ func newExperimentContext(
resyncPeriod time.Duration,
enqueueExperimentAfter func(obj any, duration time.Duration),
) *experimentContext {

exCtx := experimentContext{
ex: experiment,
templateRSs: templateRSs,
Expand Down Expand Up @@ -416,6 +415,19 @@ func (ec *experimentContext) reconcileAnalysisRun(analysis v1alpha1.ExperimentAn
eventType = corev1.EventTypeWarning
}
ec.recorder.Eventf(ec.ex, record.EventOptions{EventType: eventType, EventReason: "AnalysisRun" + string(newStatus.Phase)}, msg)
meeech marked this conversation as resolved.
Show resolved Hide resolved

// Handle the case where the Analysis Run belongs to an Experiment, and the Experiment is a Step in the Rollout
// This makes sure the rollout gets the Analysis Run events, which will then trigger any subscribed notifications
// #4009
roRef := experimentutil.GetRolloutOwnerRef(ec.ex)
if roRef != nil {
rollout, err := ec.argoProjClientset.ArgoprojV1alpha1().Rollouts(ec.ex.Namespace).Get(context.TODO(), roRef.Name, metav1.GetOptions{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am ok leaving this as a client call because the rest of the experiment code does this but it would be nice if we had access to the rollouts informer in experiment controller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'd be open to making that change (maybe later iteration - i think i will be back in this code later around fixing the complation/delay bug I reported). I wasn't sure about the implications around a changelike that, so I took a light touch.

if err != nil {
ec.log.Warnf("Failed to get parent Rollout of the Experiment '%s': %v", roRef.Name, err)
} else {
ec.recorder.Eventf(rollout, record.EventOptions{EventType: corev1.EventTypeWarning, EventReason: "AnalysisRun" + string(newStatus.Phase)}, msg)
}
}
}
experimentutil.SetAnalysisRunStatus(ec.newStatus, *newStatus)
}()
Expand Down Expand Up @@ -627,7 +639,6 @@ func (ec *experimentContext) assessAnalysisRuns() (v1alpha1.AnalysisPhase, strin

// newAnalysisRun generates an AnalysisRun from the experiment and template
func (ec *experimentContext) newAnalysisRun(analysis v1alpha1.ExperimentAnalysisTemplateRef, args []v1alpha1.Argument, dryRunMetrics []v1alpha1.DryRun, measurementRetentionMetrics []v1alpha1.MeasurementRetention, analysisRunMetadata *v1alpha1.AnalysisRunMetadata) (*v1alpha1.AnalysisRun, error) {

if analysis.ClusterScope {
analysisTemplates, clusterAnalysisTemplates, err := ec.getAnalysisTemplatesFromClusterAnalysis(analysis)
if err != nil {
Expand Down Expand Up @@ -772,7 +783,6 @@ func (ec *experimentContext) getAnalysisTemplatesFromRefs(templateRefs *[]v1alph
templates = append(templates, innerTemplates...)
}
}

}
uniqueTemplates, uniqueClusterTemplates := analysisutil.FilterUniqueTemplates(templates, clusterTemplates)
return uniqueTemplates, uniqueClusterTemplates, nil
Expand Down
9 changes: 9 additions & 0 deletions utils/experiment/experiment.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ import (

var terminateExperimentPatch = []byte(`{"spec":{"terminate":true}}`)

func GetRolloutOwnerRef(experiment *v1alpha1.Experiment) *metav1.OwnerReference {
for _, owner := range experiment.OwnerReferences {
if owner.Kind == "Rollout" {
return &owner
}
}
return nil
}

func HasFinished(experiment *v1alpha1.Experiment) bool {
return experiment.Status.Phase.Completed()
}
Expand Down
62 changes: 59 additions & 3 deletions utils/experiment/experiment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,65 @@ import (
"github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/fake"
)

func TestGetRolloutOwnerRef(t *testing.T) {
t.Run("no owner references", func(t *testing.T) {
e := &v1alpha1.Experiment{}
ownerRef := GetRolloutOwnerRef(e)
assert.Nil(t, ownerRef)
})

t.Run("non-rollout owner reference", func(t *testing.T) {
e := &v1alpha1.Experiment{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{
{
Kind: "Deployment",
Name: "deploy",
},
},
},
}
ownerRef := GetRolloutOwnerRef(e)
assert.Nil(t, ownerRef)
})

t.Run("multiple owner references with rollout", func(t *testing.T) {
rolloutOwner := metav1.OwnerReference{
Kind: "Rollout",
Name: "rollout",
}
e := &v1alpha1.Experiment{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{
{
Kind: "Deployment",
Name: "deploy",
},
rolloutOwner,
},
},
}
ownerRef := GetRolloutOwnerRef(e)
assert.Equal(t, &rolloutOwner, ownerRef)
})

t.Run("only rollout owner reference", func(t *testing.T) {
rolloutOwner := metav1.OwnerReference{
Kind: "Rollout",
Name: "rollout",
}
e := &v1alpha1.Experiment{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{
rolloutOwner,
},
},
}
ownerRef := GetRolloutOwnerRef(e)
assert.Equal(t, &rolloutOwner, ownerRef)
})
}

func TestHasFinished(t *testing.T) {
e := &v1alpha1.Experiment{}
assert.False(t, HasFinished(e))
Expand Down Expand Up @@ -42,7 +101,6 @@ func TestCalculateTemplateReplicasCount(t *testing.T) {
Status: v1alpha1.TemplateStatusFailed,
})
assert.Equal(t, int32(0), CalculateTemplateReplicasCount(e, template))

}

func TestPassedDurations(t *testing.T) {
Expand Down Expand Up @@ -70,7 +128,6 @@ func TestPassedDurations(t *testing.T) {
e.Status.AvailableAt = &metav1.Time{Time: now.Add(-2 * time.Second)}
passedDuration, _ = PassedDurations(e)
assert.True(t, passedDuration)

}

func TestGetTemplateStatusMapping(t *testing.T) {
Expand Down Expand Up @@ -113,7 +170,6 @@ func TestReplicaSetNameFromExperiment(t *testing.T) {
}

func TestExperimentByCreationTimestamp(t *testing.T) {

now := metav1.Now()
before := metav1.NewTime(metav1.Now().Add(-5 * time.Second))

Expand Down
Loading