cephfs: add clone progress report in clone status

[Nikhil-Ladha]

Add clone progress report in clone status output
Relevant Ceph PR: ceph/ceph#54620
Ceph Tracker: https://tracker.ceph.com/issues/61904

Checklist

• Added tests for features and functional changes
• Public functions and types are documented
• Standard formatting is applied to Go code
• Is this a new API? Added a new file that begins with //go:build ceph_preview
• Ran make api-update to record new APIs

New or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.

The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with @Mergifyio rebase to rebase your PR when github indicates that the PR is out of date with the base branch.

[Nikhil-Ladha]

Ref issue: ceph/ceph-csi#4813

[phlogistonjohn]

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.

[Nikhil-Ladha]

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?

[phlogistonjohn]

Tests as in that the json/struct is populated correctly

That would be fine thanks.

[Nikhil-Ladha]

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.

[Nikhil-Ladha]

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

[phlogistonjohn]

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!

[phlogistonjohn]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[Nikhil-Ladha]

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?

[phlogistonjohn]

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.

[Nikhil-Ladha]

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

[anoopcs9]

Can you please link the relevant ceph change in PR description for future reference?

[Nikhil-Ladha]

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

[anoopcs9]

lgtm, thanks.

[phlogistonjohn]

New test looks mostly good.

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!).

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?

[Nikhil-Ladha]

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.

[phlogistonjohn]

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.

[Nikhil-Ladha]

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.

[phlogistonjohn]

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

Yes please

Pull request has been modified.

[Nikhil-Ladha]

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

Yes please

Done

[phlogistonjohn]

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.

[Nikhil-Ladha]

Ok, added both the tests.
Please check now.

[anoopcs9]

Since we are going with two different test files I am gonna suggest the following names:

• cephfs/admin/clone_progress_report_test.go with main build tag.
• cephfs/admin/clone_progress_report_unsupported_test.go with !main build tag.

[anoopcs9]

We'll have to make changes with the build tags as and when the corresponding backports are processed and merged. In case we forget nightly CI runs will remind us by reporting failures I guess.