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

(gce) added upsert google scaling policy atomic operation #743

Merged
merged 1 commit into from
Jul 18, 2016

Conversation

danielpeach
Copy link
Contributor

(gce) added upsert google scaling policy atomic operation
to be followed by delete operation.

@duftler @lwander PTAL


def scalingPolicies
if (isRegional) {
scalingPolicies = compute.regionAutoscalers().list(project, region).execute().getItems()
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be reading these from the cache

Copy link

Choose a reason for hiding this comment

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

I would have expected this to behave similarly to CopyLastGoogleServerGroupAtomicOperation. In that operation, we fetch the server group from the cache by using GoogleClusterProvider. Then we retrieve the Autoscaler (and all other) attributes from the server group model itself. Any reason not to do the same thing here? Should be able to reuse the same GCEUtil.queryServerGroup() call.

Copy link

Choose a reason for hiding this comment

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

Ah, you're already making this call. Any reason to trust the other attributes of serverGroup but not the autoscaler bits?

@danielpeach danielpeach force-pushed the gce-upsert-scaling-policy branch 3 times, most recently from 87fe60d to 68717e2 Compare July 15, 2016 06:15
@@ -36,6 +36,7 @@ import com.netflix.spinnaker.clouddriver.google.deploy.exception.GoogleOperation
import com.netflix.spinnaker.clouddriver.google.deploy.exception.GoogleResourceNotFoundException
import com.netflix.spinnaker.clouddriver.google.model.GoogleDisk
import com.netflix.spinnaker.clouddriver.google.model.GoogleDiskType
import com.netflix.spinnaker.clouddriver.google.model.GoogleAutoscalingPolicy
Copy link

Choose a reason for hiding this comment

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

Sort order is off here.

@@ -574,10 +575,9 @@ class GCEUtil {
}
}

return new Autoscaler(name: serverGroupName,
zone: migCreateOperation.zone,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The autoscaler is properly upserted even if zone (or region) is omitted.

Copy link

Choose a reason for hiding this comment

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

Please add this as a comment in the code.

@duftler
Copy link

duftler commented Jul 18, 2016

Please add a comment to the PR describing how you tested this (other than the unit tests).

@duftler
Copy link

duftler commented Jul 18, 2016

LGTM after addressing comments.

@danielpeach
Copy link
Contributor Author

The PR has been manually tested with the curl commands in UpsertGoogleAutoscalingPolicyAtomicOperation for both zonal and regional server groups.

The commands test the following:

1). An autoscaler can be created for a server group with the specified properties.
2). Properties of an autoscaler can be updated without specifying all properties of the policy.
3). Properties of an autoscaler can be removed (e.g. it's possible to remove cpuUtilization, loadBalancingUtilization, and customMetricUtilizations fields).

@danielpeach danielpeach merged commit 4001665 into spinnaker:master Jul 18, 2016
ttomsu pushed a commit to ttomsu/clouddriver that referenced this pull request Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants