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

Fix issue with duplicate resources - backwards compatibility #416

Closed
wants to merge 5 commits into from

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Apr 18, 2023

Feature or Bugfix

  • Bugfix

Detail

  • In v1.5.0 SageMaker Studio base infra is moved out in a nested stack. As a consequence sometimes there are conflicts with already existing AWS resources that the nested stack is trying to re-create. But the issue is bigger than that, the new nested stack would create a new Sagemaker domain and delete the previous one. If there are existing Sagemaker studio users this results in errors because it cannot delete a domain with existing users. We have 2 alternatives: 1) migrate existing users to the new profile 2) revert the changes and put the Sagemaker resources inside the environment stack, this will keep the previous resources.

Alternative 1 is quite complex (and I am not sure it is even possible). The benefit that it brings, which is "logical separation of environment-module resources" is not worth the effort.

Relates

#409

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dlpzx dlpzx requested review from AmrSaber and nikpodsh April 18, 2023 07:27
@dlpzx dlpzx requested a review from AmrSaber April 18, 2023 08:27
from ...utils.cdk_nag_utils import CDKNagUtil
from ...utils.runtime_stacks_tagging import TagsUtil

logger = logging.getLogger(__name__)


class SageMakerDomain(NestedStack):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should we delete SageMakerDomain ? If we decided not to go with the nested stack and rollback it, I'd still leave this class, remove only NestedStack, changed init to create_ml_studio_part` (or something like that) and keep using it :)

@dlpzx
Copy link
Contributor Author

dlpzx commented Apr 18, 2023

I think I am going to start clean in another branch with your ideas, with no rush. Thanks @nikpodsh

@dlpzx dlpzx closed this Apr 18, 2023
@dlpzx dlpzx deleted the v1m5m0-fixes branch April 18, 2023 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants