Auto sync storage profiles #378 #382

UmmulkiramR · 2023-11-15T21:36:05Z

Score-client will now dynamically load storage profile specific implementations (currently S3 or Azure) based on the profile score-server is running on. This will eliminate the need for users to ensure that score-client is running on a profile compatible with
score-server.

This change contains

a new endpoint to fetch the profiles currently active on score-server (current storage profiles on score-server are azure, amazon, collaboratory).
configuration changes to ensure the right storage implementations are injected at runtime for uploads and downloads based on the score-server profiles fetched.
defaulting behaviour when used with older score-server versions where the profile endpoint isn't available. There the score-client will use the default (S3) storage profile specific implementation.

joneubank

Thanks for the quick turn around on this!

Most important thing I'm requesting here is the response data in the /profiles endpoint. I think we need to be returning information strictly about the storage type for the server, and not exposing all our runtime profiles. The client will then need to correctly map the storage type to its own corresponding profiles/beans.

One additional request: Can we please update the documentation in the score-client README.md . Currently there is text about using a profile to connect with azure, we now replace this with text that explains that the client will request the storage profile from the server and use the correct functionality, but when the score server is version {current version} or lower then the Azure profile is necessary.

joneubank · 2023-11-20T15:35:50Z

score-server/src/main/java/bio/overture/score/server/controller/BaseController.java

+
+  @RequestMapping(method = RequestMethod.GET, value = "/profile")
+  public List<String> getProfile(@RequestHeader(HttpHeaders.AUTHORIZATION) final String accessToken) {
+      return Arrays.asList(environment.getActiveProfiles());


Two thoughts here:

We don't want to reveal to the public all our runtime profiles

We should have separation between our runtime profile names and the storage type that we are telling the client to operate in.

So can we create an enum in score-core that can be shared between client and server. This can list the storage types that score supports (S3, Azure). This method should return only one of these enum values, and the client can then make its switch based on that value, instead of having to look through a list and do string checks.

joneubank · 2023-11-20T15:38:01Z

score-client/src/main/resources/application.yml

+  level:
+    bio.overture.score.client: DEBUG


I don't think we want to leave this in our release.

joneubank · 2023-11-20T15:38:52Z

score-client/src/main/java/bio/overture/score/client/util/BeanUtil.java

+   public Object getBeanForProfile(Class cl){
+     List<String> profileList = appContext.getBean("profiles", List.class);
+      if(profileList.isEmpty()){
+        profileList.add("collaboratory");


Why are we adding this profile by default?

joneubank · 2023-11-20T15:46:31Z

score-client/src/main/java/bio/overture/score/client/command/UploadCommand.java

+  private BeanUtil beanUtil;
+
+  @PostConstruct
+  public void initializeStorageProfile() throws Exception{


Code formatting Exception {.

Looks like score does not have a code formatting plugin... the rest of the overture apps do, maybe we can bring that in here... I'll open another ticket to look into this. For reference, we have used coveoos plugin called fmt-maven-plugin, and it looks like this has moved to spotify now:

https://github.com/spotify/fmt-maven-plugin

joneubank · 2023-11-20T15:48:05Z

score-client/src/main/java/bio/overture/score/client/config/AzureConfig.java

+// Note: Name the beans as profile-name+class-type to be able to dynamically fetch beans at runtime based on score-server profiles
 @Configuration
-@Profile("azure")
 public class AzureConfig {

  @Bean
-  public UploadService uploadService() {
+  public AzureUploadService azureUploadService(){


Why do we need to provide the exact named class instead of an implementation of the parent class (UploadService). The note says it has to be done but I don't really understand why, what's the reason/justification?

joneubank · 2023-11-20T16:03:07Z

score-server/src/main/java/bio/overture/score/server/controller/BaseController.java

+    Environment environment;
+
+  @RequestMapping(method = RequestMethod.GET, value = "/profile")
+  public List<String> getProfile(@RequestHeader(HttpHeaders.AUTHORIZATION) final String accessToken) {


This auth header is not being used, but is still listed as required. I don't think this endpoint needs to be authorized, should we remove the header param?

… a single profile value. The actual profile name and the profile value returned by the api are now different.

UmmulkiramR · 2023-11-22T15:02:27Z

Thanks for the quick turn around on this!

Most important thing I'm requesting here is the response data in the /profiles endpoint. I think we need to be returning information strictly about the storage type for the server, and not exposing all our runtime profiles. The client will then need to correctly map the storage type to its own corresponding profiles/beans.

One additional request: Can we please update the documentation in the score-client README.md . Currently there is text about using a profile to connect with azure, we now replace this with text that explains that the client will request the storage profile from the server and use the correct functionality, but when the score server is version {current version} or lower then the Azure profile is necessary.

yes, I forgot to mention that the API only returns information regarding the storage type. I have added a new profile configuration (in the server) that will handle that part.

joneubank · 2023-11-22T20:20:04Z

score-client/README.md

+
+Score-client dynamically loads storage profile specific implementations (currently S3 or Azure) based on the profile score-server is running on.
+It does this by calling an endpoint on score-server to fetch the active storage profiles.
+For older score-server versions where the profile endpoint isn't available, the score-client will use the default (S3) storage profile implementation.


Can I still specify the azure profile on my client for interacting with older servers that are azure based?

joneubank · 2023-11-22T20:21:27Z

score-client/src/main/java/bio/overture/score/client/config/ProfileConfig.java

+  }
+
+  private String getStorageProfile() {
+    if(isTest){ return "s3"; }


Can we use a mock of this method/class instead of hard coding values for tests?

joneubank · 2023-11-22T20:22:07Z

score-client/src/main/java/bio/overture/score/client/storage/kf/KFStorageClient.java

@@ -75,7 +75,8 @@ private String getKFDownloadEndpoint(String objectId){

  @SneakyThrows
  private <T> ResponseEntity<T> getResponse(Class<T> responseType, String accessToken, String url){
-    val entity = new HttpEntity<T>(null, buildAuthHeader(accessToken));
+    //val entity = new HttpEntity<T>(null, buildAuthHeader(accessToken));


Remove commented code.

joneubank · 2023-11-22T20:31:47Z

score-server/src/main/java/bio/overture/score/server/config/ProfileConfig.java

+
+  @Bean
+  String activeStorageProfile() {
+    Map<String, String> storageProfiles = Map.of("collaboratory", "s3", "azure", "az", "test", "test");


test is here twice.

Can we return a known enum value instead of an arbitrary String? There are really only 2 values we should be returning S3 and AZURE. We can define an enum in score-core since this is a dependency used by the server and client, then we can map each of the server profiles to the corresponding enum value.

- Storage profile values now come from an enum in score-core - test configuration created to mock storage profile bean - users will be able to provide a default profile value when working with old score-server instances

…th old score-server instances

joneubank

Awesome! Thanks!

joneubank · 2023-11-24T17:06:37Z

score-server/src/main/resources/application.yml

  sigV4Enabled: true

  #amazon
-  endpoint: https://object.cancercollaboratory.org:9080 #s3-external-1.amazonaws.com
+  endpoint: s3-external-1.amazonaws.com


joneubank · 2023-11-24T17:07:49Z

score-core/src/main/java/bio/overture/score/core/model/StorageProfiles.java

+import java.util.HashSet;
+import java.util.Set;
+
+public enum StorageProfiles {


* Readme updated * mergeback for 5.9.1-SNAPSHOT * updated readme copy * updated readme copy * Update README.md Co-authored-by: Jon Eubank <[email protected]> * CORS configurations for score server allowing multiple origins at once - #367 * Organizing score-server application.yml * docker JRE ireplacing JRE alpine image to support multiple architecturesmage multi arch supported * latest ubuntu LTS 22.04 score client * using JDK image as builder * update docker dind (#376) from docker image docker:18.06-dind to docker:20.10-dind * fix docker dind TLS (#379) * increase Jenkins timeout (#380) from 30 to 45 mins * Fix/jenkins extend timeout (#381) * increase Jenkins timeout from 30 to 45 mins * increase Jenkins timeout from 45 to 60 mins * increase jenkins timeout * update DeployWithHelm job name in Jenkinsfile (#383) * fix for issue #385 (#386) Co-authored-by: UmmulkiramR <[email protected]> * Auto sync storage profiles #378 (#382) * added server endpoint to get profiles * change to switch score-client storage implementations * cleared azure and s3 related entries app.yml * some bug fixes and enhancements * updated comment * added test profile. * added test profile. * refactored code based on review comments - BaseController now returns a single profile value. The actual profile name and the profile value returned by the api are now different. * debug logging removed * replaced profile value * updated readme * updated readme * added a test profile * review changes - Storage profile values now come from an enum in score-core - test configuration created to mock storage profile bean - users will be able to provide a default profile value when working with old score-server instances * users will be able to provide a default profile value when working with old score-server instances * config change * added a test config in score server --------- Co-authored-by: UmmulkiramR <[email protected]> * rc release. Includes - auto sync storage profiles - #387 - azure download fails - #385 - fix in Jenkins to build the Score docker image to run on amd/arm architectures and updating score-client image to use latest LTS Ubuntu- #374 --------- Co-authored-by: Mitchell Shiell <[email protected]> Co-authored-by: dahiyaAD <[email protected]> Co-authored-by: dahiyaAD <[email protected]> Co-authored-by: Jon Eubank <[email protected]> Co-authored-by: Leonardo Rivera <[email protected]> Co-authored-by: UmmulkiramR <[email protected]>

UmmulkiramR added 5 commits November 15, 2023 11:26

added server endpoint to get profiles

fd4989c

change to switch score-client storage implementations

40c5642

cleared azure and s3 related entries app.yml

0523a5f

some bug fixes and enhancements

54d8a37

updated comment

8246cee

UmmulkiramR requested a review from joneubank November 15, 2023 21:55

UmmulkiramR added 2 commits November 17, 2023 12:03

added test profile.

9a3182e

added test profile.

5b1b716

joneubank requested changes Nov 20, 2023

View reviewed changes

joneubank reviewed Nov 20, 2023

View reviewed changes

UmmulkiramR added 4 commits November 20, 2023 15:06

refactored code based on review comments - BaseController now returns…

2680a8c

… a single profile value. The actual profile name and the profile value returned by the api are now different.

debug logging removed

6e57b9c

replaced profile value

aff9bde

updated readme

2a347ca

UmmulkiramR requested a review from joneubank November 20, 2023 21:05

UmmulkiramR added 2 commits November 20, 2023 16:08

updated readme

e4681c9

added a test profile

616da15

UmmulkiramR closed this Nov 22, 2023

UmmulkiramR reopened this Nov 22, 2023

joneubank reviewed Nov 22, 2023

View reviewed changes

UmmulkiramR added 4 commits November 23, 2023 14:36

review changes

2ee609c

- Storage profile values now come from an enum in score-core - test configuration created to mock storage profile bean - users will be able to provide a default profile value when working with old score-server instances

users will be able to provide a default profile value when working wi…

b1cf219

…th old score-server instances

config change

11a5806

added a test config in score server

aa7815d

UmmulkiramR requested a review from joneubank November 24, 2023 16:20

joneubank approved these changes Nov 24, 2023

View reviewed changes

UmmulkiramR merged commit 7c2fdce into develop Nov 24, 2023
3 checks passed

UmmulkiramR deleted the auto_sync_storage_profiles_#378 branch November 24, 2023 17:14

UmmulkiramR linked an issue Nov 24, 2023 that may be closed by this pull request

Auto sync score-client to operate on same storage profile as score-server #378

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto sync storage profiles #378 #382

Auto sync storage profiles #378 #382

UmmulkiramR commented Nov 15, 2023 •

edited

Loading

joneubank left a comment

joneubank Nov 20, 2023

joneubank Nov 20, 2023

UmmulkiramR Nov 20, 2023

joneubank Nov 20, 2023

joneubank Nov 20, 2023

joneubank Nov 20, 2023

joneubank Nov 20, 2023

UmmulkiramR commented Nov 22, 2023

joneubank Nov 22, 2023

joneubank Nov 22, 2023

joneubank Nov 22, 2023

joneubank Nov 22, 2023

joneubank left a comment

joneubank Nov 24, 2023

joneubank Nov 24, 2023

Auto sync storage profiles #378 #382

Auto sync storage profiles #378 #382

Conversation

UmmulkiramR commented Nov 15, 2023 • edited Loading

joneubank 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

UmmulkiramR commented Nov 22, 2023

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

joneubank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UmmulkiramR commented Nov 15, 2023 •

edited

Loading