Fix sync when the pod gets re-created

[johnmcollier]

What type of PR is this?
/kind bug

What does does this PR do / why we need it:
This PR fixes the issue seen in #2807, where if something in the devfile changed that resulted in a new pod, the source code would get lost (as the emptyDir volume went away). Odo wouldn't detect this and wouldn't do a full sync, only a sync of the files that changed on the user's disk.

To fix this, if the component already exists, this PR compares the pod name before and after the call to a.createOrUpdateComponent. If the pod names are different, the pod has been re-created and a full sync is needed.

Which issue(s) this PR fixes:

Fixes #2807

How to test changes / Special notes to the reviewer:

1. Build my branch
2. Create a devfile component
3. Run odo push
4. Make a change in the devfile to cause a new pod to get created
5. Run odo push again, verify the entire source code is where it should be on the container(s)

@maysunfaisal is working on integration tests for devfile exec that will have this scenario covered

[maysunfaisal]

We can probably use the pod retrieved after creating component?

[maysunfaisal]

Can we just use the pod retrieved above after createOrUpdateComponent and rollout check https://github.com/openshift/odo/pull/2811/files#diff-825a1a88d75e789899c58e89a79eb767R73?

Because that is essentially the new pod, we dont necessarily need to retrieve it again here. You can probably skip the if condition if componentExists and initialize podName := "" and you wont need this retrieval here.

[johnmcollier]

The object returned from WaitForDeploymentRollout is a Deployment which we can't get the pod name from directly, unfortunately.

[maysunfaisal]

yep sorry, my bad. I like the change and i confirmed the fix, thanks! 👍

[maysunfaisal]

@johnmcollier #2735 was merged, you will need to rebase the PR

[codecov]

Codecov Report

Merging #2811 into master will increase coverage by 0.04%.
The diff coverage is 51.72%.

@@            Coverage Diff             @@
##           master    #2811      +/-   ##
==========================================
+ Coverage   43.46%   43.50%   +0.04%     
==========================================
  Files          94       94              
  Lines        8601     8618      +17     
==========================================
+ Hits         3738     3749      +11     
- Misses       4512     4518       +6     
  Partials      351      351              

Impacted Files	Coverage Δ
...g/devfile/adapters/kubernetes/component/adapter.go	26.94% <36.36%> (+1.43%)	⬆️
pkg/kclient/pods.go	43.83% <100.00%> (+2.40%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd8416c...1986c98. Read the comment docs.

[amitkrout]

@maysunfaisal is working on integration tests for devfile exec that will have this scenario covered

@johnmcollier ok, make sense. @maysunfaisal do we have an open issue for this? or are you planning to send the test pr directly ? ATM i would say let create an open issue for integration test for devfile exec and mark this pr test may be in the task list of devfile exec test overage, so that we won't forget to add this scenario. WDYT ?

[amitkrout]

/test v4.3-integration-e2e-benchmark

[kadel]

To fix this, if the component already exists, this PR compares the pod name before and after the call to a.createOrUpdateComponent. If the pod names are different, the pod has been re-created and a full sync is needed.

As long as we are using Recreate strategy using the pod names should work.
But wouldn't it be safer to use metadata.generation to detect if there was an update?

/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [kadel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[maysunfaisal]

@amitkrout I have opened an issue here #2818

[johnmcollier]

@kadel Good question! I was finding that the metadata.generation field was still getting updated, even if nothing in the spec field of the deployment was changing, so it wasn't viable.

(On a side note: I'm note sure why the generation field would change when nothing in the spec changed, but that's what I observed)

Johns-MacBook-Pro-3:openLiberty johncollier$ kubectl get deploy openliberty -o jsonpath='{.metadata.generation}{"\n"}'
6
Johns-MacBook-Pro-3:openLiberty johncollier$ odo push
 •  Push devfile component openliberty  ...
 ✓  Waiting for component to start [44ms]
 ✓  Checking file changes for pushing [3ms]
 ✓  No file changes detected, skipping build. Use the '-f' flag to force the build.
 ✓  Push devfile component openliberty [509ms]
 ✓  Changes successfully pushed to component
Johns-MacBook-Pro-3:openLiberty johncollier$ kubectl get deploy openliberty -o jsonpath='{.metadata.generation}{"\n"}'
7

[kadel]

we need to cover this scenario with integration test

[maysunfaisal]

fixes look good, thx for the change
/lgtm

[johnmcollier]

@kadel Yeah, agreed. Maysun's been working on integration tests for the devfile exec scenario and already has this scenario covered in his tests, so that's why I left it out of this PR.

[maysunfaisal]

yep thats how this issue was discovered.

[maysunfaisal]

/test ci/prow/v4.3-integration-e2e-benchmark

[maysunfaisal]

/test v4.3-integration-e2e-benchmark

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.