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

Only recreate subscriptions in Parallels when necessary #7961

Closed
Cali0707 opened this issue Jun 3, 2024 · 7 comments · Fixed by #7965
Closed

Only recreate subscriptions in Parallels when necessary #7961

Cali0707 opened this issue Jun 3, 2024 · 7 comments · Fixed by #7965
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature-request triage/accepted Issues which should be fixed (post-triage)

Comments

@Cali0707
Copy link
Member

Cali0707 commented Jun 3, 2024

Problem
Currently, the reconciler for the Parallel resource deletes and creates a new subscription whenever there is a need for a change in a subscription because some fields in subscriptions are immutable. However, many fields are mutable and in those cases we should be doing an Update instead.

Exit Criteria
A unit test showing that the susbcriptions are updated instead of deleted and recreated

Time Estimate (optional):
How many developer-days do you think this may take to resolve? 1

Additional context (optional)
Add any other context about the feature request here.

  • A similar fix for the Sequence resource is here: fix: sequence updates subscription where possible, instead of recreating it #7948
  • The relevant reconciler code for the Parallel is here:
    func (r *Reconciler) reconcileSubscription(ctx context.Context, branchNumber int, expected *messagingv1.Subscription) (*messagingv1.Subscription, error) {
    sub, err := r.subscriptionLister.Subscriptions(expected.Namespace).Get(expected.Name)
    // If the resource doesn't exist, we'll create it.
    if apierrs.IsNotFound(err) {
    sub = expected
    logging.FromContext(ctx).Infof("Creating subscription: %+v", sub)
    newSub, err := r.eventingClientSet.MessagingV1().Subscriptions(sub.Namespace).Create(ctx, sub, metav1.CreateOptions{})
    if err != nil {
    // TODO: Send events here, or elsewhere?
    //r.Recorder.Eventf(p, corev1.EventTypeWarning, subscriptionCreateFailed, "Create Parallel's subscription failed: %v", err)
    return nil, fmt.Errorf("failed to create Subscription Object for branch: %d : %s", branchNumber, err)
    }
    return newSub, nil
    } else if err != nil {
    logging.FromContext(ctx).Errorw("Failed to get Subscription", zap.Error(err))
    // TODO: Send events here, or elsewhere?
    //r.Recorder.Eventf(p, corev1.EventTypeWarning, subscriptionCreateFailed, "Create Parallels's subscription failed: %v", err)
    return nil, fmt.Errorf("failed to get Subscription: %s", err)
    } else if !equality.Semantic.DeepDerivative(expected.Spec, sub.Spec) {
    // Given that spec.channel is immutable, we cannot just update the subscription. We delete
    // it instead, and re-create it.
    err = r.eventingClientSet.MessagingV1().Subscriptions(sub.Namespace).Delete(ctx, sub.Name, metav1.DeleteOptions{})
    if err != nil {
    logging.FromContext(ctx).Infow("Cannot delete Subscription", zap.Error(err))
    return nil, err
    }
    newSub, err := r.eventingClientSet.MessagingV1().Subscriptions(sub.Namespace).Create(ctx, expected, metav1.CreateOptions{})
    if err != nil {
    logging.FromContext(ctx).Infow("Cannot create Subscription", zap.Error(err))
    return nil, err
    }
    return newSub, nil
    }
    return sub, nil
    }
@Cali0707
Copy link
Member Author

Cali0707 commented Jun 3, 2024

/good-first-issue

Copy link

knative-prow bot commented Jun 3, 2024

@Cali0707:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@knative-prow knative-prow bot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Jun 3, 2024
@7h3-3mp7y-m4n
Copy link
Contributor

hey, I would like to work on this

@babugeet
Copy link
Contributor

babugeet commented Jun 3, 2024

@7h3-3mp7y-m4n : if you are not able to work on it pls let me know
I would like to work on it
/assign

@7h3-3mp7y-m4n
Copy link
Contributor

7h3-3mp7y-m4n commented Jun 3, 2024

I was hoping to work on this and I waited for a member to assign me, I was even trying to reach them through Slack to get more info

@babugeet
Copy link
Contributor

babugeet commented Jun 3, 2024

@7h3-3mp7y-m4n I will unassing myself
/unassign

@babugeet
Copy link
Contributor

babugeet commented Jun 4, 2024

@7h3-3mp7y-m4n : let me know if you want me to take this up

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature-request triage/accepted Issues which should be fixed (post-triage)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants