Implement SCM Pull LRO by samdgupi · Pull Request #15364 · cdapio/cdap

samdgupi · 2023-10-16T17:28:45Z

Long running operation implementation for SCM pull. It reuses most of the existing git operation logic in InMemorySourceControlOperationRunner which will be refactored when we remove the previous task based implementation.

The actual deploy and mark latest operations are exposed via a new helper interface which will be implemented when the following will be merged
#15336
#15340

samdgupi · 2023-10-16T18:49:34Z

+        deployed.add(appTobeDeployed.get());
+        updateMetadata.accept(getOperationMeta());
+      });
+    } catch (Exception e) {


Catch Exception as we should convert any and all exception to error

albertshau · 2023-10-17T23:48:21Z

+      // If error happened we try to delete all deployed versions
+      for (ApplicationId appId : deployed) {
+        try {
+          helper.deleteAppVersion(appId);


If we're deleting, we shouldn't do it one by one. That being said, I don't think we need to (or should) delete, as there is no concept of deleting just a single app version today.

We do have appLifecycleService.removeApplicationVersion which I was referring ton but looks like the public API AppLifecycleHttpHandler.deleteAppVersion is deprecated.
The main concern is without deletion this versions left would be surfaced to the user in history creating confusion. Specially in case of a pull of new application as the application would have only one version which is not marked latest.

this isn't the right way to prevent these versions from surfacing in the history for a couple reasons:

It's always possible to see it after the app is added, but before it is marked as latest

The delete can fail

If we are concerned about showing it, the right way to handle it is to add something like an isHidden flag. Trying to delete it here is like a half solution, so I don't think we need to bother doing it.

I agree. Rather than adding a additional flag I think we can have a enum based state field which can be extended later and covers both latest status and hidden status. I have added a followup Jira for this as this is not a blocker for the core feature.

albertshau · 2023-10-17T23:51:32Z

+/**
+ * Request type for {@link PullOperation}.
+ */
+public class PullOperationRequest {


should name it BulkPullRequest or something like that -- want to differentiate between bulk and single operations

The plan is we would migrate the single pull to also use the same operation so BulkPull might be misleading.

albertshau · 2023-10-17T23:52:25Z

+  public static final class AppToPull {
+
+    private final String application;
+    private final String currentGitHash;


is the hash the commit hash or something else? If it's the commit hash, shouldn't that be a top level property and not specific to each app?

As mentioned offline the hash is git-hash of each app config file not the commitid/hash. Hence the property is per app.

albertshau · 2023-10-17T23:57:27Z

 */
 public class OperationMeta {
  private final Set<OperationResource> resources;
+  @Nullable


why is this nullable? Shouldn't it always have a start time?

albertshau · 2023-10-17T23:58:02Z

+/**
+ * Thrown when the application config file is not present in the repository.
+ */
+public class ApplicationNotFoundException extends NotFoundException {


why do we need this when there is already an ApplicationNotFoundException?

The two excpetions has to different semantical meaning. I have changed the name to GitAppConfigNotFoundException

albertshau · 2023-10-17T23:59:53Z

+  private final Set<ApplicationId> deployed;
+
+  @Inject
+  PullOperation(InMemorySourceControlOperationRunner scmOpRunner, SourceControlHelper helper) {


shouldn't this be a SourceControlOperationRunner instead of the InMemory one?

As mentioned in the description for easy reuse of existing code we are putting the new multi push and pull logic in InMemorySourceControlOperationRunner, rather than refactoring out into a new class, which will be done in future.

The PullOperation would always use InMemorySourceControlOperationRunner as the remote runner just creates a task which we do not want.

The SourceControlOperationRunner naming is a bit confusing at this point as the operation here does not refer to LongRunningOperation but git operations.

samdgupi · 2023-10-20T11:04:13Z

    if (request.getFilter().getOperationType() != null) {
      startFields.add(Fields.stringField(StoreDefinition.OperationRunsStore.TYPE_FIELD,
-          request.getFilter().getOperationType())
+          request.getFilter().getOperationType().toString())


@samdgupi to check if we should use .name() instead

serialization should use .name(), assuming we are using .valueOf() to deserialize.

This field is not use for serialisation but to index the status separately.

albertshau · 2023-10-20T22:05:57Z

    RepositoryConfig repoConfig = getRepositoryMeta(appRef.getParent()).getConfig();
    SourceControlMeta latestMeta = store.getAppSourceControlMeta(appRef);
-    PullAppResponse<?> pullResponse = sourceControlOperationRunner.pull(new PulAppOperationRequest(appRef, repoConfig));
+    PullAppResponse<?> pullResponse = sourceControlOperationRunner.pull(new PullAppOperationRequest(appRef, repoConfig));


please make sure all checkstyle issues are addressed

albertshau · 2023-10-20T22:07:28Z

+   *     concrete implementation is because the git operations should always run inMemory.
+   * @param helper provides utilities to provide app-fabric exposed functionalities.
+   */
+  public PullAppsOperation(@Assisted PullAppsRequest request,


don't need this annotation

Forgot the Inject annotation. We want the runner and helper to be injected by guice based on where the operation is running but the caller to pass the request only.

The other option is to inject runner/helper in the caller to avoid assisted injection. But that will make the caller be aware of all possible operation inputs that need to be injected

I see, that makes more sense. Though the constructor should be package private if we're using injection.

albertshau · 2023-10-20T22:32:57Z

+ * Provides various helper methods for source control operations. Would be implemented for both
+ * running in app-fabric and running in workers.
+ */
+public interface SourceControlHelper {


it should be divided by functionality (ex: separate application and repository interfaces) so that it is more modular and re-usable. We don't really have these types of util or helper classes anywhere in the code because they tend to accumulate random things that don't really have to do with each other.

albertshau · 2023-10-20T22:36:59Z

    if (request.getFilter().getOperationType() != null) {
      startFields.add(Fields.stringField(StoreDefinition.OperationRunsStore.TYPE_FIELD,
-          request.getFilter().getOperationType())
+          request.getFilter().getOperationType().toString())


serialization should use .name(), assuming we are using .valueOf() to deserialize.

albertshau · 2023-10-20T22:48:22Z

+    OperationError err = operation.run(mockContext).get();
+
+    Assert.assertNull(err);
+    Mockito.verify(mockHelper, Mockito.times(4)).deployAppVersion(Mockito.any(), Mockito.any());


In general, we try to avoid using Mockito when it is not needed, especially if it is being used to mock our own classes. It is generally more difficult to understand, and it tends to encourage tests that break when the implementation of a class changes instead of testing the actual APIs contracts. For example, if we ever added a way to deploy multiple apps in a single call, this test would break even if the class was functioning properly. Or if we change it not to use this helper, the test would need to be rewritten.

It would be better if the run() method returned something more useful, like the collection of OperationResources it added.

Updated the LongRunningOperation interface and checking the returned resources

albertshau · 2023-10-20T22:57:59Z

+        String versionId = RunIds.generate().getId();
+        appTobeDeployed.set(new ApplicationId(context.getRunId().getNamespace(),
+            response.getApplicationName(), versionId));
+        helper.deployAppVersion(


keep in mind that an operation needs to be idempotent in case the system dies and needs to resume the operation on restart. So while there is nothing technically wrong with running through the entire request again, it would be better to look up the operation resources first to see if a good chunk of it is already done.

This can be an improvement added in a future PR, but please be sure to add a jira and TODO for it.

Though this also directly contradicts with the fact that it appears like the RepositoryConfig is allowed to change in between calls.

The exact behavior should be clearly defined somewhere, at least in the javadocs. I'm not sure how we want it to behave in all these edge cases.

Making this idempotent is hard as you mentioned the repsoitory config and/or the git state can change (force push). I will add a TODO as an future investigation improvement.

Also I do not think we have any requirements to add ability to resume for git operations.

The current expected behaviour I will add in the javadocs. We would also need to update the user documentaion/guide.

albertshau · 2023-10-20T23:04:57Z

  }

  @Override
  public ListenableFuture<OperationError> run(LongRunningOperationContext context) {


normally the future type is whatever the return value of the corresponding blocking call would be, with any errors thrown as exceptions. It would make more sense for this to return information about all the applications that were deployed, or something along those lines.

The reason I am returning OperationError is to support operations with partial failures i.e if the operation wants to continue processing the whole batch and accumulate any error happening for each resources. Throwing exception does not feel like a good model.

We don't have any usage for returning the operated/created resources here. As we want to know exactly what resources were changed in case the runtime crashed, we want to update the resources in operation metadata if and when the resource is changed through the updateResources method in the LongRunningOperationContext

Updated the interface based on discussion.

albertshau · 2023-10-20T23:08:14Z

+    Mockito.verify(mockHelper, Mockito.times(1)).markAppVersionsLatest(Mockito.any());
+    // As runIds are generated inside operation we cannot actually check
+    // the generated resource uris in metadata
+    Assert.assertEquals(4, gotResource.size());


this should be comparing the exact contents, not just the collection size.

albertshau · 2023-10-20T23:11:27Z

+/**
+ * Thrown when the application config file is not present in the repository.
+ */
+public class GitAppConfigNotFoundException extends NotFoundException {


Git -> SourceControl

albertshau · 2023-10-20T23:11:57Z


  @Inject
-  InMemorySourceControlOperationRunner(RepositoryManagerFactory repoManagerFactory) {
+  public InMemorySourceControlOperationRunner(RepositoryManagerFactory repoManagerFactory) {


injected constructors should almost never be public to prevent manual constructor outside of unit tests.

This was needed to construct InMemorySourceControlOperationRunner in PullOperation test. I will try to find a better way

albertshau

few minor comments

albertshau · 2023-10-25T20:56:14Z

+   *     concrete implementation is because the git operations should always run inMemory.
+   * @param helper provides utilities to provide app-fabric exposed functionalities.
+   */
+  public PullAppsOperation(@Assisted PullAppsRequest request,


I see, that makes more sense. Though the constructor should be package private if we're using injection.

albertshau · 2023-10-25T20:58:59Z

+
+    // TODO(samik, CDAP-20855) Investigate and implement the cleanup for created versions in case of error
+
+    // TODO(samik) Update this after along with the runner implementation


is this a todo for the near future (current release) or for a future release? If it will be here beyond the current release, it would be better to make the interface a blocking one instead of pretending it is asynchronous. Then make everything async in some future PR.

(either way, this can be done in a follow up PR)

albertshau · 2023-10-25T21:01:13Z

-    new RemoteExceptionProvider<>(NotFoundException.class, NotFoundException::new);
+      new RemoteExceptionProvider<>(NotFoundException.class, NotFoundException::new);
+  private final RemoteExceptionProvider<SourceControlAppConfigNotFoundException> gitAppNotFoundExceptionProvider =
+    new RemoteExceptionProvider<>(SourceControlAppConfigNotFoundException.class, SourceControlAppConfigNotFoundException::new);


samdgupi changed the base branch from develop to operation-api October 16, 2023 18:46

samdgupi commented Oct 16, 2023

View reviewed changes

samdgupi requested review from albertshau, arjan-bal, de-lan and tivv October 16, 2023 18:56

samdgupi added build Triggers github actions build 6.10 labels Oct 17, 2023

albertshau reviewed Oct 17, 2023

View reviewed changes

samdgupi force-pushed the pull-operation branch 2 times, most recently from 509abdf to b281ac8 Compare October 20, 2023 04:52

samdgupi changed the base branch from operation-api to develop October 20, 2023 08:56

samdgupi force-pushed the pull-operation branch from b281ac8 to 1b00ba0 Compare October 20, 2023 10:58

samdgupi commented Oct 20, 2023

View reviewed changes

Comment thread ...-app-fabric/src/test/java/io/cdap/cdap/internal/app/sourcecontrol/PullAppsOperationTest.java Outdated

samdgupi requested a review from albertshau October 20, 2023 11:09

samdgupi marked this pull request as ready for review October 20, 2023 11:09

albertshau reviewed Oct 20, 2023

View reviewed changes

samdgupi self-assigned this Oct 23, 2023

samdgupi force-pushed the pull-operation branch 3 times, most recently from 2976dce to 4a575fe Compare October 24, 2023 19:00

samdgupi requested a review from albertshau October 24, 2023 19:04

albertshau approved these changes Oct 25, 2023

View reviewed changes

samdgupi force-pushed the pull-operation branch 4 times, most recently from 48e37d2 to c789ca9 Compare October 26, 2023 10:00

CDAP-20854-add-pull-operation

a56a6a9

samdgupi force-pushed the pull-operation branch from c789ca9 to a56a6a9 Compare October 26, 2023 14:14

samdgupi merged commit de4a284 into develop Oct 26, 2023

samdgupi deleted the pull-operation branch October 26, 2023 16:03


		// TODO(samik, CDAP-20855) Investigate and implement the cleanup for created versions in case of error

		// TODO(samik) Update this after along with the runner implementation

Conversation

samdgupi commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samdgupi Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samdgupi Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samdgupi Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samdgupi Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samdgupi Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

samdgupi commented Oct 16, 2023 •

edited

Loading

samdgupi Oct 18, 2023 •

edited

Loading

samdgupi Oct 18, 2023 •

edited

Loading

samdgupi Oct 18, 2023 •

edited

Loading

samdgupi Oct 23, 2023 •

edited

Loading

samdgupi Oct 24, 2023 •

edited

Loading