Announce maintenance in stable deployments (#5979) #6095

achave11-ucsc · 2024-03-26T18:05:44Z

Connected issues: (#5979)

Checklist

Author

PR is a draft
Target branch is develop
Name of PR branch matches issues/<GitHub handle of author>/<issue#>-<slug>
On ZenHub, PR is connected to all issues it (partially) resolves
PR description links to connected issues
PR title matches¹ that of a connected issue _{or comment in PR explains why they're different}
PR title references all connected issues
For each connected issue, there is at least one commit whose title references that issue

¹ when the issue title describes a problem, the corresponding PR
title is Fix: followed by the issue title

Author (partiality)

Added p tag to titles of partial commits
This PR is labeled partial _{or completely resolves all connected issues}
This PR partially resolves each of the connected issues _{or does not have the partial label}

Author (chains)

This PR is blocked by previous PR in the chain _{or is not chained to another PR}
The blocking PR is labeled base _{or this PR is not chained to another PR}
This PR is labeled chained _{or is not chained to another PR}

Author (reindex, API changes)

Added r tag to commit title _{or the changes introduced by this PR will not require reindexing of any deployment}
This PR is labeled reindex:dev _{or the changes introduced by it will not require reindexing of dev}
This PR is labeled reindex:anvildev _{or the changes introduced by it will not require reindexing of anvildev}
This PR is labeled reindex:anvilprod _{or the changes introduced by it will not require reindexing of anvilprod}
This PR is labeled reindex:prod _{or the changes introduced by it will not require reindexing of prod}
This PR is labeled reindex:partial and its description documents the specific reindexing procedure for dev, anvildev, anvilprod and prod _{or requires a full reindex or carries none of the labels reindex:dev, reindex:anvildev, reindex:anvilprod and reindex:prod}
This PR and its connected issues are labeled API _{or this PR does not modify a REST API}
Added a (A) tag to commit title for backwards (in)compatible changes _{or this PR does not modify a REST API}
Updated REST API version number in app.py _{or this PR does not modify a REST API}

Author (upgrading deployments)

Ran make image_manifests.json and committed the resulting changes _{or this PR does not modify azul_docker_images, or any other variables referenced in the definition of that variable}
Documented upgrading of deployments in UPGRADING.rst _{or this PR does not require upgrading deployments}
Added u tag to commit title _{or this PR does not require upgrading deployments}
This PR is labeled upgrade _{or does not require upgrading deployments}
This PR is labeled deploy:shared _{or does not modify image_manifests.json, and does not require deploying the shared component for any other reason}
This PR is labeled deploy:gitlab _{or does not require deploying the gitlab component}
This PR is labeled deploy:runner _{or does not require deploying the runner image}

Author (hotfixes)

Added F tag to main commit title _{or this PR does not include permanent fix for a temporary hotfix}
Reverted the temporary hotfixes for any connected issues _{or the none of the stable branches (anvilprod and prod) have temporary hotfixes for any of the issues connected to this PR}

Author (before every review)

Rebased PR branch on develop, squashed old fixups
Ran make requirements_update _{or this PR does not modify requirements*.txt, common.mk, Makefile and Dockerfile}
Added R tag to commit title _{or this PR does not modify requirements*.txt}
This PR is labeled reqs _{or does not modify requirements*.txt}
make integration_test passes in personal deployment _{or this PR does not modify functionality that could affect the IT outcome}

Peer reviewer (after approval)

PR is not a draft
Ticket is in Review requested column
PR is awaiting requested review from system administrator
PR is assigned to only the system administrator

System administrator (after approval)

Actually approved the PR
Labeled connected issues as demo or no demo
Commented on connected issues about demo expectations _{or all connected issues are labeled no demo}
Decided if PR can be labeled no sandbox
A comment to this PR details the completed security design review
PR title is appropriate as title of merge commit
N reviews label is accurate
Moved connected issues to Approved column
PR is assigned to only the operator

Operator (before pushing merge the commit)

System administrator

Background migrations for dev.gitlab are complete _{or this PR is not labeled deploy:gitlab}
Background migrations for anvildev.gitlab are complete _{or this PR is not labeled deploy:gitlab}
PR is assigned to only the operator

Operator (before pushing merge the commit)

Operator (chain shortening)

Changed the target branch of the blocked PR to develop _{or this PR is not labeled base}
Removed the chained label from the blocked PR _{or this PR is not labeled base}
Removed the blocking relationship from the blocked PR _{or this PR is not labeled base}
Removed the base label from this PR _{or this PR is not labeled base}

Operator (after pushing the merge commit)

Operator (reindex)

Operator

Propagated the deploy:shared, deploy:gitlab, deploy:runner, reindex:partial, reindex:anvilprod and reindex:prod labels to the next promotion PRs _{or this PR carries none of these labels}
Propagated any specific instructions related to the deploy:shared, deploy:gitlab, deploy:runner, reindex:partial, reindex:anvilprod and reindex:prod labels, from the description of this PR to that of the next promotion PRs _{or this PR carries none of these labels}
PR is assigned to no one

Shorthand for review comments

L line is too long
W line wrapping is wrong
Q bad quotes
F other formatting problem

scripts/manage_maintenance.py

src/azul/maintenance.py

coveralls · 2024-08-15T18:33:11Z

coverage: 84.937% (-0.4%) from 85.355%
when pulling 9af2251 on issues/achave11-ucsc/5979-maintenance-notice
into be8a0bc on develop.

codecov · 2024-08-15T18:35:34Z

Codecov Report

Attention: Patch coverage is 41.23711% with 114 lines in your changes missing coverage. Please review.

Project coverage is 84.91%. Comparing base (be8a0bc) to head (9af2251).

Files	Patch %	Lines
src/azul/maintenance.py	41.23%	114 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #6095      +/-   ##
===========================================
- Coverage    85.33%   84.91%   -0.42%     
===========================================
  Files          156      157       +1     
  Lines        20748    20948     +200     
===========================================
+ Hits         17706    17789      +83     
- Misses        3042     3159     +117

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/azul/maintenance.py

scripts/manage_maintenance.py

nadove-ucsc

The box is checked for

Updated REST API version number in app.py or this PR does not modify a REST API

However this PR does modify the REST API and does not update the version.

nadove-ucsc · 2024-08-22T18:54:09Z

scripts/manage_maintenance.py

+
+    service = MaintenanceService()
+
+    if args.command == "list":


nadove-ucsc · 2024-08-22T18:57:40Z

scripts/manage_maintenance.py

+            active = events.active_event()
+            active = {} if active is None else {'active': active.to_json()}
+            pending = events.pending_events()
+            pending = {'pending': list(pe.to_json() for pe in pending)}


Suggested change

pending = {'pending': list(pe.to_json() for pe in pending)}

pending = {'pending': [pe.to_json() for pe in pending]}

nadove-ucsc · 2024-08-22T19:02:01Z

scripts/manage_maintenance.py

+    elif args.command == "adjust":
+        events = service.adjust_event(parse_duration(args.duration))
+    else:
+        assert False, 'Invalid command'


An AssertionError is not the proper exception to raise here. I believe this should be:

Suggested change

assert False, 'Invalid command'

raise argparse.ArgumentError(subparsers, 'Invalid command')

nadove-ucsc · 2024-08-22T19:47:40Z

scripts/manage_maintenance.py

+    azul.RequirementError: Try a duration such as "2d 6hrs", "1.5 Days", or "15m"
+    """
+
+    pattern = r'(\d*\.?\d+)\s*(d|h|m|s)'


d|h|m|s can be written more clearly and succinctly as [dhms]

That said, capturing only the 1st character of the unit name troubles me. Consider what would happen if we encountered input such as "100 milliseconds", "1 month", or "1.5 squirrels".

(these are all good ideas for additional doctests)

"1.5 Days Bad foo" is another example of an input that is accepted and should not be.

nadove-ucsc · 2024-08-22T19:50:57Z

scripts/manage_maintenance.py

+    pattern = r'(\d*\.?\d+)\s*(d|h|m|s)'
+    matches = re.findall(pattern, duration.lower())
+    require(bool(matches), 'Try a duration such as "2d 6hrs", "1.5 Days", or "15m"')
+    time_delta = {'days': 0, 'hours': 0, 'minutes': 0, 'seconds': 0}


Might be a good place to use defaultdict?

nadove-ucsc · 2024-08-22T20:06:59Z

src/azul/maintenance.py

+        events = enumerate(self.events)
+        for i, e in events:


This looks peculiar. Is there a reason for not writing

Suggested change

events = enumerate(self.events)

for i, e in events:

for i, e in enumerate(self.events):

?

nadove-ucsc · 2024-08-22T20:07:55Z

src/azul/maintenance.py

+            self.events = events
+            raise
+
+    def adjust_event(self, additional_duration: timedelta):


Suggested change

def adjust_event(self, additional_duration: timedelta):

def adjust_event(self, additional_duration: timedelta) -> MaintenanceEvent:

nadove-ucsc · 2024-08-22T20:11:30Z

src/azul/maintenance.py

+        schedule.validate()
+        return schedule
+
+    def put_schedule(self, schedule: MaintenanceSchedule):


Needs return type annotation. I think JSON is satisfactory, but please verify that.

The return value also isn't used anywhere, so you could remove it.

nadove-ucsc · 2024-08-22T20:12:00Z

src/azul/maintenance.py

+                        degraded: list[str] | None = None,
+                        unavailable: list[str] | None = None) -> MaintenanceEvent:
+        """
+        Uses the given inmput parametes to provision a new MaintenanceEvent.


Suggested change

Uses the given inmput parametes to provision a new MaintenanceEvent.

Uses the given input parameters to provision a new MaintenanceEvent.

nadove-ucsc · 2024-08-22T20:18:17Z

lambdas/service/app.py

+    }
+)
+def get_maintenance_schedule():
+    service = MaintenanceService()


MaintenanceService has a cached_property which is made pointless if we construct a new instance of the service for every request.

Normally the service would be cached on the controller, which would be cached on the app. We don't have a controller here, but I still think it would e a good idea to find a way to reuse this instance.

nadove-ucsc

Clicked the wrong button.

Initial patch by Hannes

github-actions bot added the orange [process] Done by the Azul team label Mar 26, 2024

github-advanced-security bot found potential problems Mar 26, 2024

View reviewed changes

scripts/manage_maintenance.py Fixed Show fixed Hide fixed

hannes-ucsc force-pushed the develop branch 3 times, most recently from 0762297 to 3f7f6ce Compare May 28, 2024 06:58

achave11-ucsc force-pushed the issues/achave11-ucsc/5979-maintenance-notice branch from 393914b to 46e1199 Compare August 15, 2024 17:47

achave11-ucsc added the API API change affecting callers label Aug 15, 2024

github-advanced-security bot found potential problems Aug 15, 2024

View reviewed changes

src/azul/maintenance.py Fixed Show fixed Hide fixed

src/azul/maintenance.py Fixed Show fixed Hide fixed

src/azul/maintenance.py Fixed Show fixed Hide fixed

src/azul/maintenance.py Fixed Show fixed Hide fixed

src/azul/maintenance.py Fixed Show fixed Hide fixed

achave11-ucsc force-pushed the issues/achave11-ucsc/5979-maintenance-notice branch 2 times, most recently from ed40514 to f994d5d Compare August 15, 2024 18:11

achave11-ucsc force-pushed the issues/achave11-ucsc/5979-maintenance-notice branch from f994d5d to 1852aec Compare August 16, 2024 01:52

github-advanced-security bot found potential problems Aug 16, 2024

View reviewed changes

src/azul/maintenance.py Fixed Show fixed Hide fixed

src/azul/maintenance.py Fixed Show fixed Hide fixed

achave11-ucsc force-pushed the issues/achave11-ucsc/5979-maintenance-notice branch 3 times, most recently from 20ca591 to df39a94 Compare August 20, 2024 17:14

hannes-ucsc force-pushed the develop branch from f54b36e to d769f27 Compare August 20, 2024 22:09

achave11-ucsc force-pushed the issues/achave11-ucsc/5979-maintenance-notice branch 2 times, most recently from 5c24f7a to 5a94830 Compare August 22, 2024 00:03

github-advanced-security bot found potential problems Aug 22, 2024

View reviewed changes

scripts/manage_maintenance.py Fixed Show fixed Hide fixed

achave11-ucsc force-pushed the issues/achave11-ucsc/5979-maintenance-notice branch 4 times, most recently from 1e63694 to 9916081 Compare August 22, 2024 07:28

achave11-ucsc requested a review from nadove-ucsc August 22, 2024 17:21

achave11-ucsc assigned nadove-ucsc Aug 22, 2024

nadove-ucsc reviewed Aug 22, 2024

View reviewed changes

nadove-ucsc self-requested a review August 22, 2024 20:20

nadove-ucsc requested changes Aug 22, 2024

View reviewed changes

nadove-ucsc removed their assignment Aug 22, 2024

hannes-ucsc and others added 2 commits August 26, 2024 09:27

[1/2] Announce maintenance in stable deployments (#5979)

36083d9

Initial patch by Hannes

[a 2/2] Announce maintenance in stable deployments (#5979)

91bca25

achave11-ucsc force-pushed the issues/achave11-ucsc/5979-maintenance-notice branch 2 times, most recently from 8479c97 to 67fe1bf Compare August 26, 2024 16:35

fixup! [a 2/2] Announce maintenance in stable deployments (#5979)

9af2251

achave11-ucsc force-pushed the issues/achave11-ucsc/5979-maintenance-notice branch from 67fe1bf to 9af2251 Compare August 26, 2024 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Announce maintenance in stable deployments (#5979) #6095

Announce maintenance in stable deployments (#5979) #6095

achave11-ucsc commented Mar 26, 2024 •

edited

Loading

coveralls commented Aug 15, 2024 •

edited

Loading

codecov bot commented Aug 15, 2024 •

edited

Loading

nadove-ucsc left a comment

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc Aug 22, 2024

nadove-ucsc left a comment

	pending = {'pending': list(pe.to_json() for pe in pending)}
	pending = {'pending': [pe.to_json() for pe in pending]}

	assert False, 'Invalid command'
	raise argparse.ArgumentError(subparsers, 'Invalid command')

	events = enumerate(self.events)
	for i, e in events:
	for i, e in enumerate(self.events):

	def adjust_event(self, additional_duration: timedelta):
	def adjust_event(self, additional_duration: timedelta) -> MaintenanceEvent:

	Uses the given inmput parametes to provision a new MaintenanceEvent.
	Uses the given input parameters to provision a new MaintenanceEvent.

Announce maintenance in stable deployments (#5979) #6095

Are you sure you want to change the base?

Announce maintenance in stable deployments (#5979) #6095

Conversation

achave11-ucsc commented Mar 26, 2024 • edited Loading

Checklist

Author

Author (partiality)

Author (chains)

Author (reindex, API changes)

Author (upgrading deployments)

Author (hotfixes)

Author (before every review)

Peer reviewer (after approval)

System administrator (after approval)

Operator (before pushing merge the commit)

System administrator

Operator (before pushing merge the commit)

Operator (chain shortening)

Operator (after pushing the merge commit)

Operator (reindex)

Operator

Shorthand for review comments

coveralls commented Aug 15, 2024 • edited Loading

codecov bot commented Aug 15, 2024 • edited Loading

Codecov Report

nadove-ucsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadove-ucsc left a comment

Choose a reason for hiding this comment

achave11-ucsc commented Mar 26, 2024 •

edited

Loading

coveralls commented Aug 15, 2024 •

edited

Loading

codecov bot commented Aug 15, 2024 •

edited

Loading