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.

Sign up for GitHub

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

Oort get server groups #1680

Merged
merged 13 commits into from
Oct 12, 2017
Merged

Oort get server groups #1680

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
40aba97
Add new clouddriver getServerGroup endpoint
andrewbackes Oct 9, 2017
eef8445
Add new clouddriver getServerGroup endpoint
andrewbackes Oct 9, 2017
f18489a
Removed frigga from oortHelper
andrewbackes Oct 9, 2017
ff6022c
Rename oortService method for clearity
andrewbackes Oct 9, 2017
09a4ae7
Merge branch 'master' into oort-get-server-groups
andrewbackes Oct 10, 2017
2e81c24
Overload getServerGroups and deprecate old prototype
andrewbackes Oct 11, 2017
8a3096c
Update test
andrewbackes Oct 11, 2017
fd2dcbd
Merge branch 'oort-get-server-groups' of https://github.com/armory-io…
andrewbackes Oct 11, 2017
6eaca21
Merge branch 'master' into oort-get-server-groups
andrewbackes Oct 11, 2017
fe2f033
Also deprecate on the DelegatedOortService
andrewbackes Oct 11, 2017
5e78cc1
Merge branch 'oort-get-server-groups' of https://github.com/armory-io…
andrewbackes Oct 11, 2017
52fb40e
Merge branch 'master' into oort-get-server-groups
lwander Oct 11, 2017
7d5d94d
Merge branch 'master' into oort-get-server-groups
andrewbackes Oct 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...ddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/DelegatingOortService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ public Response getServerGroup(String app, String account, String region, String
return getService().getServerGroup(app, account, region, serverGroup);
}

@Override
public Response getServerGroup(String account, String serverGroup, String region) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe flag the prior function with @Deprecated as well here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good find. Missed that one. Made the change here: https://github.com/spinnaker/orca/pull/1680/files#diff-044981bc49bd67385ca8beec17533501R48

Copy link
Member

Choose a reason for hiding this comment

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

Sweet let's merge

return getService().getServerGroup(account, serverGroup, region);
}

@Override
public Response getTargetServerGroup(String app, String account, String cluster, String cloudProvider, String scope, String target) {
return getService().getTargetServerGroup(app, account, cluster, cloudProvider, scope, target);
Expand Down
12 changes: 9 additions & 3 deletions orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/OortService.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,17 @@ interface OortService {
@Query("region") String region,
@Path("cloudProvider") String cloudProvider)

@Deprecated
@GET("/applications/{app}/serverGroups/{account}/{region}/{serverGroup}")
Response getServerGroup(@Path("app") String app,
@Path("account") String account,
@Path("region") String region,
@Path("serverGroup") String serverGroup)
@Path("account") String account,
@Path("region") String region,
@Path("serverGroup") String serverGroup)

@GET("/serverGroups/{account}/{region}/{serverGroup}")
Response getServerGroup(@Path("account") String account,
@Path("region") String region,
@Path("serverGroup") String serverGroup)

@GET("/applications/{app}/clusters/{account}/{cluster}/{cloudProvider}/{scope}/serverGroups/target/{target}")
Response getTargetServerGroup(@Path("app") String app,
Expand Down
11 changes: 1 addition & 10 deletions ...louddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/OortHelper.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package com.netflix.spinnaker.orca.clouddriver.utils

import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.frigga.Names
import com.netflix.spinnaker.orca.clouddriver.OortService
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroup
import org.springframework.beans.factory.annotation.Autowired
Expand Down Expand Up @@ -59,15 +58,7 @@ class OortHelper {
String serverGroupName,
String location,
String cloudProvider) {
def name = Names.parseName(serverGroupName)
return convertedResponse(List) { oortService.getServerGroupFromCluster(name.app, account, name.cluster, serverGroupName, null, cloudProvider) }
.map({ List<Map> serverGroups ->
serverGroups.find {
it.region == location || it.zones?.contains(location) || it.namespace == location
}
}).map({ Map serverGroup ->
new TargetServerGroup(serverGroup)
})
return new TargetServerGroup(convert(oortService.getServerGroup(account, location, serverGroupName) , Map))
}

public <T> Optional<T> convertedResponse(Class<T> type, Closure<Response> request) {
Expand Down
1 change: 0 additions & 1 deletion .../com/netflix/spinnaker/orca/kato/tasks/rollingpush/WaitForNewUpInstancesLaunchTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ class WaitForNewUpInstancesLaunchTask implements RetryableTask {

// similar check in `AbstractInstancesCheckTask`
def response = oortService.getServerGroup(
stageData.application,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is backwards incompatible as it breaks the API contract with CloudDriver (unless you update CloudDriver, too). In my opinion this should not have been merged as written.

Why not try the application specific endpoint after a 404? The app specific endpoint is marked as deprecated (thank you) and can be logged out as such. That would give folks some time to update CloudDriver (perhaps as part of a blessed release).

I will probably put up a PR for that if it seems reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

You should (ideally) be installing Spinnaker from a list of verified versions. We publish those to avoid having to add all the complexity of supporting multiple endpoints for several releases.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've been trying to stick with the verified versions, but due to various issues across Spinnaker services we inevitably need to update to newer versions while we keep other services set at the verified version. I agree that ideally we could always stick with the verified versions, but as of yet that has not been the case.

Copy link
Member

Choose a reason for hiding this comment

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

When issues come up can you file them in spinnaker/spinnaker/issues? This way we can help keep Spinnaker as up-to-date as possible by publishing verified cross-component releases.

Copy link
Contributor

@jonsie jonsie Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sure. In that same note - how do we prod you guys for a new verified release? Currently 1.4.2 features a number of bugs around Orca pipeline templates and we can't make any progress until we update both Orca and CloudDriver, which we are hesitant to do because we continue to bump into problems (like this one) in our testing environment.

The fundamental problem I see with trying to bundle Spinnaker verified releases is that it's treated like a monolithic release, whereas Spinnaker is really a set of microservices. If semver would be taken a bit more seriously, problems like this wouldn't come about (or maybe, come about less) and we could treat Spinnaker like a set of microservices (with confidence) rather than waiting on the "verified release".

Copy link
Contributor

Choose a reason for hiding this comment

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

@jonsie here's a good overview from @jtk54 about the release process for verified releases if you're interested: https://www.youtube.com/watch?v=aQ1lhi1l18s

stageData.account,
stage.context.region as String,
stage.context.asgName as String
Expand Down
2 changes: 1 addition & 1 deletion .../netflix/spinnaker/orca/kato/tasks/rollingpush/WaitForNewUpInstancesLaunchTaskSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class WaitForNewUpInstancesLaunchTaskSpec extends Specification {
def response = task.execute(stage)

then:
1 * oortService.getServerGroup(application, account, region, serverGroup) >> oortResponse
1 * oortService.getServerGroup(account, region, serverGroup) >> oortResponse
response.status == expectedStatus


Expand Down