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

Ability to auto-reapply bundles #558

Closed
wants to merge 10 commits into from

Conversation

ibrokethecloud
Copy link
Contributor

The PR introduces a new boolean field in BundleDeploymentOptions alwaysReapply

Users can use this flag to trigger re-application of modified bundled on downstream clusters.

When this field is set, the fleet-agent will trigger redeployment of the modified bundle at a fixed interval to avoid overloading the api server.

@leodotcloud
Copy link

Might be good to expose the interval too via config? I am pretty sure someone will reach out again in future to make it configurable :)

@ibrokethecloud ibrokethecloud marked this pull request as ready for review October 15, 2021 00:30
@nickgerace
Copy link
Contributor

#566

@skaven81
Copy link

I agree with @leodotcloud that the timing should be exposed via config as well.

Perhaps as a separate feature request, it would be good to have this be fully configurable rather than just a Boolean.

For example, have the following configuration options:

reapplyPolicy:
  applyDelay: <timespec> (default=1m)
  maxRetries: <int> (default=0)
  backoff: <timespec> (default=0)

To get the default behavior today, something like this would be the default settings:

reapplyPolicy:
  maxRetries: 0   # don't retry if it fails; applyDelay is irrelevant.  backoff=0 is default, so will never reset and try again.

If you want to get the alwaysReapply: true behavior:

reapplyPolicy:
  maxRetries: -1  # infinite; always retry
  applyDelay: 1m

This would make it always retry applying, every minute, forever.

Then you could also create a middle-of-the-road configuration:

reapplyPolicy:
  maxRetries: 3
  applyDelay: 3m
  backoff: 1h

This would have the agent attempt to resolve the changes three times, waiting 3 minutes between each retry. If the last retry fails, it will wait an hour and then reset the retries counter and go at it again.

@nickgerace
Copy link
Contributor

nickgerace commented Oct 21, 2021

@ibrokethecloud I'd be comfortable with some form of the above feedback from @skaven81 if you have the cycles. Apply delay does not seem as useful to me if we have max backoff and max retries, though I do see the use if the idea is not to hammer an external API (though, I'd argue you have other problems in that case).

I would still prefer to have "resync" exist as a boolean though since we want an intuitive solution here. While us here understand that -1 means "infinite", not all users will. I'd say maintaining resync and adding options may be the best path forward.

@ibrokethecloud
Copy link
Contributor Author

@nickgerace added additional logic to custom reapply settings

Users can now either choose the standard resync behaviour, by specifying the fleld resync:true. In this scenario fleet agent will try and reapply the bundle contents every 5 minutes.

namespace: default
name: demo1
labels:
  app: demo1
resync: true  
targets:
- clusterSelector: {}  

The users can also customize the behavior as shown below

namespace: default
name: demo1
labels:
  app: demo1
resync: true  
resyncPolicy:
  maxRetries: 2
  resyncDelay: 2m
  backoffDelay: 3m
targets:
- clusterSelector: {}  

If users want unlimited max tries with a custom delay

namespace: default
name: demo1
labels:
  app: demo1
resync: true  
resyncPolicy:
  maxRetries: -1
  resyncDelay: 2m
targets:
- clusterSelector: {}  

@skaven81
Copy link

This is awesome, and exactly what I was looking for. 👍

leodotcloud
leodotcloud previously approved these changes Oct 26, 2021
Copy link

@leodotcloud leodotcloud left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot Gaurav for taking care of this.

@leodotcloud
Copy link

Ping @StrongMonkey @nickgerace ... can you please review and merge this PR?

@nickgerace
Copy link
Contributor

@ibrokethecloud is this ready for re-review?

if bd.Spec.Options.Resync || bd.Spec.StagedOptions.Resync {
if bd.Spec.Options.ResyncPolicy == nil && bd.Spec.StagedOptions.ResyncPolicy == nil {
if status.LastApply == nil {
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look correct.... if lastApply is nil and resync is true then we return false? Look like it will never be deployed since that's the initail state..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be deployed.

Right now there are two handles for BundleDeployments, DeployBundle and MonitorBundle.

The DeployBundle will deploy the bundle the first time and set the lastApply timestamp. I just wanted MonitorBundle to ignore reapply / re-queueing the bundle until the first deploy has happened.

return false
}

if policy.MaxRetries == DefaultMaxRetries && status.LastApply.Add(policy.ResyncDelay.Duration).Before(time.Now()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm why do we even compare policy.MaxRetries == DefaultMaxRetries here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DefaultMaxRetries was a specific case where you specify MaxRetries as -1 which means its an infinite loop where the objects will be queued at the custom reapply interval.

@manno
Copy link
Member

manno commented Jun 6, 2023

We are planning to correct drift instead of reapplying on an interval. We might revisit this in the future.

@manno manno closed this Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants