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

Dry Run #30

Merged
merged 3 commits into from
May 30, 2022
Merged

Dry Run #30

merged 3 commits into from
May 30, 2022

Conversation

ro-tex
Copy link
Collaborator

@ro-tex ro-tex commented May 25, 2022

PULL REQUEST

Overview

Implement a dry_run cluster-level configuration option which omits all calls to skyd's pin/unpin.

The feature is a bit raw and underspecced, so feedback is welcome. We need to get it done ASAP, though, so we can kick this off in prod. We can also make this a temporary feature and remove it once the Pinner is stable and trusted.

Even if, for some reason, we decide to not go with this feature, there are changes in this PR that still need to be merged, e.g. findAndPinOneUnderpinnedSkylink being renamed to managedFindAndPinOneUnderpinnedSkylink and the lock-and-copy strat in there.

Internal discussion: https://discord.com/channels/542938080349519882/978547799799005184/979044946185183294

This depends on #27 because I needed some of the changes made there.
I'll select that branch as base for the moment, in order to make the reviews easier.

Example for Visual Changes

Checklist

Review and complete the checklist to ensure that the PR is complete before assigned to an approver.

  • All new methods or updated methods have clear docstrings
  • Testing added or updated for new methods
  • Verify if any changes impact the WebPortal Health Checks
  • Approriate documentation updated
  • Changelog file created

Issues Closed

Closes SKY-676

@ro-tex ro-tex self-assigned this May 25, 2022
@ro-tex ro-tex requested review from MSevey and ChrisSchinnerl May 25, 2022 16:32
@ro-tex ro-tex marked this pull request as ready for review May 25, 2022 16:32
@linear
Copy link

linear bot commented May 25, 2022

SKY-676 Pinner: Dry Run

We decided that we needed a dry run feature for Pinner in order to get it in prod in a safer way, observe it for a while in that very complicated environment and then enable it for real.

PR: #30

Discussion: https://discord.com/channels/542938080349519882/978547799799005184/979044946185183294

@ro-tex ro-tex requested a review from mrcnski May 25, 2022 16:55
Copy link
Contributor

@MSevey MSevey left a comment

Choose a reason for hiding this comment

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

I think this looks good. We should be able to write a quick test that verifies in dry run a skylink doesn't get pinned.

basically

trigger a pin event
wait the pin interval
verify skyd doesn't have the pin
turn off dry run
verify skyd gets the pin

Base automatically changed from ivo/post_testing_fixes to main May 25, 2022 18:27
mrcnski
mrcnski previously approved these changes May 25, 2022
Makefile Show resolved Hide resolved
@mrcnski mrcnski requested a review from MSevey May 25, 2022 21:07
Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

One possible issue I noticed with this yesterday (and somehow forgot about): if the scanner is running in dry mode, isn't it going to be picking up the same skylinks over and over, because they never get pinned? Should there be a new value like dryPinned or something to simulate marking them pinned?

@MSevey
Copy link
Contributor

MSevey commented May 26, 2022

One possible issue I noticed with this yesterday (and somehow forgot about): if the scanner is running in dry mode, isn't it going to be picking up the same skylinks over and over, because they never get pinned? Should there be a new value like dryPinned or something to simulate marking them pinned?

yes they would get picked up over and over, that is fine, it is just going to be log spam, but ideally this would die down to only the skylinks that will need to be pinned.

one of the reasons for the dry run mode was to reduce the risk of pinning a bunch of skylinks we don't need to as the cluster syncs up with the initial pinner deploy.

if we see a bunch of log spam, and then nothing, that would ultimate validate our concerns about deploying in dry run mode.

So I don't think we should have any dryPinned value.

Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

Looks good, waiting for @ro-tex to have a look and merge.

@ro-tex ro-tex mentioned this pull request May 30, 2022
@ro-tex ro-tex merged commit 8a8ad92 into main May 30, 2022
@ro-tex ro-tex deleted the ivo/dry_run branch May 30, 2022 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants