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

feat: Add immediate option to ReplicationSource #812

Closed
wants to merge 5 commits into from

Conversation

onedr0p
Copy link
Contributor

@onedr0p onedr0p commented Jul 27, 2023

Describe what this PR does

Adds immediate option to ReplicationSource - immediate can be used to stop replication when a new ReplicationSource is created. Defaults to "true".

Is there anything that requires special attention?

I am unable to run make test (Apple Silicon issue?) locally so looking for some feedback. This also could use some unit tests if required, however I am unable to write & test due to that issue until I find another way.

Related issues:
#627

Signed-off-by: Devin Buhl <devin@buhl.casa>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: onedr0p
Once this PR has been reviewed and has the lgtm label, please assign cooktheryan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2023

Hi @onedr0p. Thanks for your PR.

I'm waiting for a backube member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@openshift-ci openshift-ci bot added the size/M label Jul 27, 2023
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #812 (f74f1f5) into main (49fe3ef) will decrease coverage by 0.1%.
The diff coverage is 63.6%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #812     +/-   ##
=======================================
- Coverage   66.9%   66.9%   -0.1%     
=======================================
  Files         55      55             
  Lines       7058    7091     +33     
=======================================
+ Hits        4726    4747     +21     
- Misses      2054    2062      +8     
- Partials     278     282      +4     
Files Changed Coverage Δ
controllers/mover/syncthing/mover.go 81.2% <ø> (ø)
controllers/mover/rclone/builder.go 87.8% <40.0%> (-2.2%) ⬇️
controllers/mover/restic/builder.go 88.8% <40.0%> (-2.1%) ⬇️
controllers/mover/rsync/builder.go 89.0% <40.0%> (-2.0%) ⬇️
controllers/mover/rsynctls/builder.go 89.3% <40.0%> (-2.0%) ⬇️
controllers/mover/rclone/mover.go 71.8% <100.0%> (+0.3%) ⬆️
controllers/mover/restic/mover.go 81.6% <100.0%> (+0.1%) ⬆️
controllers/mover/rsync/mover.go 76.7% <100.0%> (+0.2%) ⬆️
controllers/mover/rsynctls/mover.go 71.1% <100.0%> (+0.2%) ⬆️
controllers/mover/syncthing/builder.go 92.9% <100.0%> (+0.1%) ⬆️

Signed-off-by: Devin Buhl <devin@buhl.casa>
onedr0p and others added 3 commits July 27, 2023 18:54
Co-authored-by: Thomas <9749173+uhthomas@users.noreply.github.com>
Signed-off-by: Devin Buhl <onedr0p@users.noreply.github.com>
Signed-off-by: Devin Buhl <devin@buhl.casa>
Signed-off-by: Devin Buhl <devin@buhl.casa>
@sonarcloud
Copy link

sonarcloud bot commented Jul 28, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
13.8% 13.8% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@tesshuflower
Copy link
Contributor

@onedr0p thanks for this PR, however I think if we do this there are a few updates we'd need to do:

  • If we add immediate to replicationsource I'd prefer to also add to replicationdestination to be consistent
  • Does it make sense to add immediate to spec.trigger somewhere? And potentially even call this something like spec.trigger.scheduleImmediate? Since I think this will not apply (and should be ignored) by manual triggers.
  • I think the actual implementation will need to be modified to set status/modify the scheduling algorithm to make sure we skip this initial sync if immediate is false. The way it's done in the PR prevents the job from starting - which would be the same effect as creating a replicationsource with paused=true. The problem with this is everything else will still happen immediately - i.e. snapshot/clone will be taken of the pvc immediately, and that pvc from snap/clone will be of the PVC at the time when the replicationsource is created. I think to do this as you intend, we'd need to modify the scheduling to not sync at all at the start which is a little more involved as we don't want to risk breaking the scheduling.

I'll discuss with the rest of the team to see if this does sound like a valid direction to go - I will try to take a look at the scheduling when I have a moment and see if it's something that could be done relatively easily.

@onedr0p
Copy link
Contributor Author

onedr0p commented Aug 24, 2023

Closing PR, will circle back if needed after the volume populator functionality is released. I still think this might be needed for other situations but there's problems with this PR as pointed out above.

@onedr0p onedr0p closed this Aug 24, 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.

3 participants