Ref issue: ceph/ceph-csi#4813

I see that this is an admin API, so it should be populating the new struct via JSON. However, I would like to see tests for this prior to approval.

I see that this is an admin API, so it should be populating the new struct via JSON. However, I would like to see tests for this prior to approval.

Tests as in that the json/struct is populated correctly or something else?

Tests as in that the json/struct is populated correctly

That would be fine thanks.

I have added a json sample for test TestParseCloneStatus under clone_test.go file, hope that helps.
I have tested locally the TestCloneSubVolumeSnapshot with fmt.Printf() statements under parseCloneStatus function and the value was propagated properly.

@phlogistonjohn can you please review it again?
TIA :)

My only remaining concern is what happens on older versions of ceph. It looks like your test only makes use of sample JSON (and that's fine) but doesn't talk to a live server so our test coverage in octopus/pacific/etc would not find an issue if this is missing or different in older versions.

Can you either affirm that you've checked octopus has this in the JSON and it parses correctly OR add a 2nd test that adds coverage by talking to the server. Thanks!

@Mergifyio rebase

rebase

✅ Branch has been successfully rebased

My only remaining concern is what happens on older versions of ceph. It looks like your test only makes use of sample JSON (and that's fine) but doesn't talk to a live server so our test coverage in octopus/pacific/etc would not find an issue if this is missing or different in older versions.

Can you either affirm that you've checked octopus has this in the JSON and it parses correctly OR add a 2nd test that adds coverage by talking to the server. Thanks!

Makes sense.
This change is merged recently and has not been backported to any releases as of now. And, looking at the ceph tracker I don't see they have plans to backport it to octopus or pacific. Only quincy and reef backport issues are created.
I will add a 2nd test that adds coverage for this api.

Quick question: How shall I make this new test run only for non-released ceph version i.e, ceph main branch? Using ceph_preview tag in the *_test.go file? Or, something else?

We've had to do some similar handling across versions, but I don't remember exactly what we've done in the past off the top of my head. Please take a look in the existing code for the various admin subpackages and you should find some examples. If you can't let me know and I'll try to find some.

Added a separate TC for the progress report api change, with the ceph_main build tag.

@anoopcs9 addressed all the concerns, please take a look now.

However, my earlier concern seems largely unaddressed. I do not remember all the Go JSON (de)serialization rules and I want to make sure that the changes in this PR do not behave poorly when executed on older ceph versions (we still support octopus!).

If the parsing wouldn't have worked then the checks on the PR would have failed because we have multiple tests that check Clone Status in their lifecycle namely TestCloneSubVolumeSnapshot and TestWorkflow. Both the TCs reached InProgress state in between. So, if they are passing for older versions in CI, I think the parsing is working fine.

An option is to take the current clone_progress_test.go file and copy it, invert the build tag (!main). and then edit the test to (a) make sure the JSON parsing works OK and (b) the values are unavailable. Sound OK?

Let me make the change and paste the results for older versions from my local setup.

However, my earlier concern seems largely unaddressed. I do not remember all the Go JSON (de)serialization rules and I want to make sure that the changes in this PR do not behave poorly when executed on older ceph versions (we still support octopus!).

If the parsing wouldn't have worked then the checks on the PR would have failed because we have multiple tests that check Clone Status in their lifecycle namely TestCloneSubVolumeSnapshot and TestWorkflow. Both the TCs reached InProgress state in between. So, if they are passing for older versions in CI, I think the parsing is working fine.

OK, that's good enough for me. I just can never remember the rules and needed reassurance without me having to run tests myself.

An option is to take the current clone_progress_test.go file and copy it, invert the build tag (!main). and then edit the test to (a) make sure the JSON parsing works OK and (b) the values are unavailable. Sound OK?

Let me make the change and paste the results for older versions from my local setup.

That'd be great - but as per the above I won't require it.

Tested for pacific:
Test output as follows:

Res:
{
  "status": {
    "state": "in-progress",
    "source": {
      "volume": "cephfs",
      "subvolume": "Jurrasic",
      "snapshot": "dinodna1",
      "group": "Park"
    }
  }
}

Res:
{
  "status": {
    "state": "complete"
  }
}

--- PASS: TestCloneProgress (7.55s)
PASS

Updated the TC as follows while testing:

        wasInProgress := false
	for done := false; !done; {
		status, err := fsa.CloneStatus(volume, group, clonename)
		assert.NoError(t, err)
		assert.NotNil(t, status)
		switch status.State {
		case ClonePending:
		case CloneInProgress:
			wasInProgress = true
			assert.Equal(t, status.ProgressReport.PercentageCloned, "")
			assert.Equal(t, status.ProgressReport.AmountCloned, "")
			assert.Equal(t, status.ProgressReport.FilesCloned, "")
		case CloneComplete:
			done = true
		case CloneFailed:
			t.Fatal("clone failed")
		default:
			t.Fatalf("invalid clone status: %q", status.State)
		}
		time.Sleep(5 * time.Millisecond)
	}
	assert.Equal(t, wasInProgress, true)

Let me know, if you would prefer to have the above changeset pushed instead of the current one.

Let me know, if you would prefer to have the above changeset pushed instead of the current one.

Yes please

Let me know, if you would prefer to have the above changeset pushed instead of the current one.

Yes please

Done

I'm sorry we must have misunderstood each other. I did not want the earlier test replaced by a !main test but in addition to it. I want both tests - two new files. Name the files differently and let the build tags determine which tests will be run.

Ok, added both the tests.
Please check now.