[v8] Max in flight flag #3085

joaopapereira · 2024-08-07T14:48:42Z

Description of the Change

Add the max-in-flight flag to all the commands

pivotalgeorge

Looks mostly-good to me. some comments are concerns, most are minor suggestions

pivotalgeorge · 2024-08-07T22:23:59Z

actor/v7pushaction/create_deployment_for_push_plan.go

-	dep.DropletGUID = pushPlan.DropletGUID
-	dep.Strategy = pushPlan.Strategy
-	dep.Relationships = resources.Relationships{constant.RelationshipTypeApplication: resources.Relationship{GUID: pushPlan.Application.GUID}}
+	dep := resources.Deployment{


[comment, no action] good improvement / we should be using this style. I wonder if our linter catches the assignment approach you've replaced

pivotalgeorge · 2024-08-07T22:26:32Z

actor/v7pushaction/setup_deployment_information_for_push_plan.go

+
+import "code.cloudfoundry.org/cli/api/cloudcontroller/ccv3/constant"
+
+func SetupDeploymentInformationForPushPlan(pushPlan PushPlan, overrides FlagOverrides) (PushPlan, error) {


[suggestion / question / nonblocking] why is this separate from create_deployment_for_push_plan

actor.PreparePushPlanSequence looked like the place where we were configuring the push plan by translating all the needed flags.

Okay makes sense (the pushplansequence). In that case what's strangely-shaped is the location (siblings within v7pushaction) of two deployment functions that both take pushPlans. I see that this parallels the situation for e.g. droplet config, so I'm happy to move on from this topic for now and leave it up to a separate refactor

pivotalgeorge · 2024-08-07T22:28:52Z

command/v7/copy_source_command.go

@@ -17,6 +17,7 @@ type CopySourceCommand struct {
 	RequiredArgs        flag.CopySourceArgs     `positional-args:"yes"`
 	usage               interface{}             `usage:"CF_NAME copy-source SOURCE_APP DESTINATION_APP [-s TARGET_SPACE [-o TARGET_ORG]] [--no-restart] [--strategy STRATEGY] [--no-wait]"`
 	Strategy            flag.DeploymentStrategy `long:"strategy" description:"Deployment strategy can be canary, rolling or null"`
+	MaxInFlight         int                     `long:"max-in-flight" default:"-1" description:"Defines the maximum number of instances that will be actively being started. Only applies when --strategy flag is specified."`


[suggestion, blocking] it feels like the default should be the same as CAPI (1) or at least a valid value (resolves to the same, since 0 and negative numbers are invalid here)

@pivotalgeorge looks like this concern has been addressed. Are you ok for this conversation to be resolved?

pivotalgeorge · 2024-08-07T22:31:44Z

command/v7/copy_source_command.go

+		return translatableerror.RequiredFlagsError{Arg1: "--max-in-flight", Arg2: "--strategy"}
+	}
+
+	if cmd.Strategy.Name != constant.DeploymentStrategyDefault && (cmd.MaxInFlight < -1 || cmd.MaxInFlight == 0) {


[suggestion, nonblocking] (cmd.MaxInFlight <= 0)

[code since changed, comment in new review]

pivotalgeorge · 2024-08-07T22:35:19Z

actor/v7pushaction/create_deployment_for_push_plan_test.go

+				}))
+			})
+
+			It("returns errors and warnings", func() {


[suggestion, nonblocking] do we want a happy-path case covered?

pivotalgeorge · 2024-08-07T22:43:26Z

command/v7/rollback_command.go

@@ -19,6 +20,7 @@ type RollbackCommand struct {
 	Version         flag.Revision `long:"version" required:"true" description:"Roll back to the specified revision"`
 	relatedCommands interface{}   `related_commands:"revisions"`
 	usage           interface{}   `usage:"CF_NAME rollback APP_NAME [--version VERSION] [-f]"`
+	MaxInFlight     int           `long:"max-in-flight" default:"-1" description:"Defines the maximum number of instances that will be actively being rolled back."`


same as previous default value comment

pivotalgeorge · 2024-08-07T22:44:08Z

command/v7/restart_command.go

 	"code.cloudfoundry.org/cli/command/v7/shared"
 )

 type RestartCommand struct {
 	BaseCommand

+	MaxInFlight         int                     `long:"max-in-flight" default:"-1" description:"Defines the maximum number of instances that will be actively restarted at any given time. Only applies when --strategy flag is specified."`


same as previous default value comment

integration/v7/isolated/continue_deployment_test.go

pivotalgeorge · 2024-08-07T22:48:13Z

resources/deployment_resource.go

@@ -22,6 +23,10 @@ type Deployment struct {
 	Strategy         constant.DeploymentStrategy
 }

+type DeploymentOpts struct {
+	MaxInFlight int `json:"max_in_flight"`


pivotalgeorge · 2024-08-07T22:52:20Z

resources/deployment_resource.go

+	if d.Options != b {
+		ccDeployment.Options = &d.Options
+		if d.Options.MaxInFlight < 1 {
+			ccDeployment.Options.MaxInFlight = 1


[concern, blocking] what happens when we talk to an older CAPI, I.e. one that doesn't have max-in-flight? in #3088 we approached this by assuming that a value of 0 was "max-in-flight unset in the response" but I'm reading this block to mean that we'll always be setting max-in-flight to the default value of 1 and including it in a POST to CAPI
edit: Unless I am missing something and this object is only for unmarshalling the CAPI response

The concern is valid. When max-in-flight gets to this point with a value we just send it to CAPI and CAPI will return an error, which I am going to address in the next commit.
Also did change the way we are defining if --max-in-flight is present or not by using a pointer instead of the -1 value which should make everything much easier to reason about.

agreed on the nullable pointer being easy to reason about. If we handle the version issue in followup work I'm happy to consider this resolved.

gururajsh

LGTM

Signed-off-by: João Pereira <[email protected]>

- Provide a better error when talking to older versions of CAPI - Address some comments in the PR review Signed-off-by: João Pereira <[email protected]>

Signed-off-by: João Pereira <[email protected]>

gururajsh

LGTM

pivotalgeorge · 2024-08-09T17:46:31Z

command/v7/copy_source_command.go

@@ -48,6 +49,14 @@ func (cmd *CopySourceCommand) ValidateFlags() error {
 		}
 	}

+	if cmd.Strategy.Name == constant.DeploymentStrategyDefault && cmd.MaxInFlight != nil {


[suggestion, blocking] I believe this is now incorrect: ~~MaxInFlight can take a value of 0, which should be invalid for this flag~~
edit: I was looking at a diff against the previous changeset version of this PR, so missed the block below. So, disregard the positive value check comment.
However, shouldn't this read && cmd.MaxInFlight == nil, as a required-arg check
-> I see that the default deployment strategy is empty-string, so this is actually correct.

pivotalgeorge

thanks for addressing previous comments
LGTM

* Allow the user to set max-in-flight while pushing app * Add max-in-flight flag to the restart command * Add max-in-flight flag to restage command * Add max-in-flight flag to rollback command * Add max-in-flight flag to copy-src command * Remove `--no-wait` to ensure no flakes --------- Signed-off-by: João Pereira <[email protected]>

* Allow the user to set max-in-flight while pushing app * Add max-in-flight flag to the restart command * Add max-in-flight flag to restage command * Add max-in-flight flag to rollback command * Add max-in-flight flag to copy-src command * Remove `--no-wait` to ensure no flakes --------- Signed-off-by: João Pereira <[email protected]> Co-authored-by: Pavel Busko <[email protected]>

pivotalgeorge mentioned this pull request Aug 7, 2024

Print max-in-flight value alongside deployment status [v8] #3088

Closed

joaopapereira requested a review from gururajsh August 7, 2024 21:24

pivotalgeorge reviewed Aug 7, 2024

View reviewed changes

gururajsh previously approved these changes Aug 8, 2024

View reviewed changes

joaopapereira added 13 commits August 8, 2024 15:40

Allow the user to set max-in-flight while pushing app

d07173f

Signed-off-by: João Pereira <[email protected]>

Add max-in-flight flag to the restart command

d0e5398

Signed-off-by: João Pereira <[email protected]>

Add max-in-flight flag to restage command

22f9462

Signed-off-by: João Pereira <[email protected]>

Add max-in-flight flag to rollback command

1925135

Signed-off-by: João Pereira <[email protected]>

Add max-in-flight flag to copy-src command

b70dcf7

Signed-off-by: João Pereira <[email protected]>

Fix issues with max-in-flight validations

62dd356

Signed-off-by: João Pereira <[email protected]>

Fix rollback and tests that check help text

ea1a89c

Signed-off-by: João Pereira <[email protected]>

Remove --no-wait to ensure no flakes

91d1c27

Signed-off-by: João Pereira <[email protected]>

Fix other integration tests

b1a0160

Signed-off-by: João Pereira <[email protected]>

Another fixed test

5d88245

Signed-off-by: João Pereira <[email protected]>

Fix order of push options

ffcb898

Signed-off-by: João Pereira <[email protected]>

Change the way we check for presence of the --max-in-flight flag

63c394b

- Provide a better error when talking to older versions of CAPI - Address some comments in the PR review Signed-off-by: João Pereira <[email protected]>

Change order of flags so they are alphabetized

deff01c

Signed-off-by: João Pereira <[email protected]>

joaopapereira dismissed gururajsh’s stale review via deff01c August 8, 2024 20:56

joaopapereira force-pushed the max-in-flight-v8 branch from 33f58ca to deff01c Compare August 8, 2024 20:56

gururajsh approved these changes Aug 9, 2024

View reviewed changes

pivotalgeorge suggested changes Aug 9, 2024

View reviewed changes

pivotalgeorge approved these changes Aug 9, 2024

View reviewed changes

joaopapereira merged commit 6c01031 into cloudfoundry:v8 Aug 9, 2024
18 checks passed

weresch mentioned this pull request Aug 12, 2024

[v8] Print max-in-flight value alongside deployment status #3097

Merged

pivotalgeorge mentioned this pull request Aug 12, 2024

Print max-in-flight value alongside deployment status [main] #3098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v8] Max in flight flag #3085

[v8] Max in flight flag #3085

joaopapereira commented Aug 7, 2024

pivotalgeorge left a comment

pivotalgeorge Aug 7, 2024

pivotalgeorge Aug 7, 2024

joaopapereira Aug 8, 2024

pivotalgeorge Aug 9, 2024

pivotalgeorge Aug 7, 2024

gururajsh Aug 9, 2024

pivotalgeorge Aug 7, 2024

pivotalgeorge Aug 9, 2024

pivotalgeorge Aug 7, 2024

pivotalgeorge Aug 7, 2024

pivotalgeorge Aug 7, 2024

pivotalgeorge Aug 7, 2024

pivotalgeorge Aug 7, 2024

joaopapereira Aug 8, 2024

pivotalgeorge Aug 9, 2024

gururajsh left a comment

gururajsh left a comment

pivotalgeorge Aug 9, 2024 •

edited

Loading

pivotalgeorge Aug 9, 2024

pivotalgeorge left a comment


		import "code.cloudfoundry.org/cli/api/cloudcontroller/ccv3/constant"

		func SetupDeploymentInformationForPushPlan(pushPlan PushPlan, overrides FlagOverrides) (PushPlan, error) {

[v8] Max in flight flag #3085

[v8] Max in flight flag #3085

Conversation

joaopapereira commented Aug 7, 2024

Description of the Change

pivotalgeorge 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

Choose a reason for hiding this comment

gururajsh left a comment

Choose a reason for hiding this comment

gururajsh left a comment

Choose a reason for hiding this comment

pivotalgeorge Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pivotalgeorge left a comment

Choose a reason for hiding this comment

pivotalgeorge Aug 9, 2024 •

edited

Loading