-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add preserve mta modules and provide rollback mechanism #1570
Conversation
@Column(name = TableColumnNames.PRESERVED_DESCRIPTOR_DESCRIPTOR, nullable = false) | ||
@Lob | ||
private byte[] descriptor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't TEXT columns be better for large texts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discuss in the chat, descriptors can have a lot of symbols and can exceed limit of TEXT datatype.
context.setVariable(Variables.DEPLOYMENT_DESCRIPTOR, preservedDescriptor.getDescriptor()); | ||
context.setVariable(Variables.MTA_MAJOR_SCHEMA_VERSION, preservedDescriptor.getDescriptor() | ||
.getMajorSchemaVersion()); | ||
context.setVariable(Variables.MTA_ARCHIVE_MODULES, preservedDescriptor.getDescriptor() | ||
.getModules() | ||
.stream() | ||
.map(Module::getName) | ||
.collect(Collectors.toSet())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you use three var-s when you could use only one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which three var's do you mean? They are set because it other steps are used for different things. Originally in blue-green process diagram, these vars are set in different steps but in rollback process diagram this can be achieved in single step
if (processOnlyUserProvidedService) { | ||
return CloudModelBuilderUtil.isUserProvidedService(resource); | ||
} | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why if processOnlyUserProvidedService is false we still return true from the method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue here is with naming of the function. In this case we are looking to include all services in the list if "processOnlyUserProvidedService" flag is not set to true. I will think about another name.
String mtaNamespace = context.getVariable(Variables.MTA_NAMESPACE); | ||
String hashedMtaNamespace = mtaNamespace != null ? MtaMetadataUtil.getHashedLabel(mtaNamespace) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do the same for the system namespace?
@@ -37,127 +37,6 @@ paths: | |||
$ref: "#/definitions/Info" | |||
security: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not delete these
@@ -403,66 +282,6 @@ securityDefinitions: | |||
flow: "password" | |||
scopes: {} | |||
definitions: | |||
AsyncUploadResult: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not delete these
Builder builder = MtaMetadataBuilder.init(deploymentDescriptor, namespace) | ||
.label(MtaMetadataLabels.SPACE_GUID, spaceGuid); | ||
|
||
Builder builder = MtaMetadataBuilder.init(deploymentDescriptor, namespace, null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is null passed instead of namespace
Builder builder = MtaMetadataBuilder.init(deploymentDescriptor, namespace) | ||
.annotation(MtaMetadataAnnotations.MTA_RESOURCE, mtaResourceAnnotation); | ||
|
||
Builder builder = MtaMetadataBuilder.init(deploymentDescriptor, namespace, null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is null passed instead of namespace
...in/java/org/cloudfoundry/multiapps/controller/core/model/BlueGreenApplicationNameSuffix.java
Show resolved
Hide resolved
@@ -33,4 +33,6 @@ | |||
<include file="/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog-1.167.0-persistence.xml" /> | |||
|
|||
<include file="/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog-1.175.0-persistence.xml" /> | |||
|
|||
<include file="/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog-1.177.0-persistence.xml" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder, update version
@@ -22,6 +23,8 @@ public class Constants { | |||
public static final String VAR_SERVICE_INSTANCE_GUID_PREFIX = "SERVICE_INSTANCE_GUID_"; | |||
public static final String UNKNOWN_LABEL = "unknown label"; | |||
public static final String UNKNOWN_PLAN = "unknown plan"; | |||
public static final String MTA_PRESERVED_NAMESPACE = "mta-preserved"; | |||
public static final String MTA_FOR_DELETION_PREFIX = "to-be-deleted"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder to update these
private void preserveDeploymentDescriptor(ProcessContext context, DeploymentDescriptor descriptor) { | ||
if (!context.getVariable(Variables.SHOULD_PRESERVE_OLD_APPS)) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be extracted in the diagram?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How? This step is used in all different type of deployment operations.
@@ -144,4 +156,45 @@ private void updateApplicationNamesInDescriptor(ProcessContext context, String s | |||
context.setVariable(Variables.DEPLOYMENT_DESCRIPTOR, descriptor); | |||
} | |||
|
|||
class RenameApplicationsForRevert implements RenameFlow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sounds like a method name, add Flow at the end?
private final DeployedMta preservedMta; | ||
private final DescriptorPreserverService descriptorPreserverService; | ||
|
||
public ApplicationsPreserveCalculator(DeployedMta deployedMta, DeployedMta preservedMta, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add some logging in this class
Preserve applications during blue green deployment and add mechanism to revert older applications in working state JIRA:LMCROSSITXSADEPLOY-3080
fc64e7f
to
9a19c8d
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Preserve applications during blue green deployment and add mechanism to revert older applications in working state
JIRA:LMCROSSITXSADEPLOY-3080