Skip online repo interaction for packaged build

[purelind]

What problem does this PR solve?

What is changed and how does it work?

Code changes

• [ ] Has Go code change
• [ ] Has CI related scripts change

Tests

• [ ] Unit test
• [ ] E2E test
• [ ] Manual test
• [ ] No code

Side effects

• [ ] Breaking backward compatibility
• [ ] Other side effects:

Related changes

• [ ] Need to cherry-pick to the release branch
• [ ] Need to update the documentation

Release Notes

Please refer to Release Notes Language Style Guide before writing the release note.

[ti-chi-bot[bot]]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign xhebox for approval. For more information see the Code Review Process. Please ensure that each of them provides their approval before proceeding.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[ottok]

@dveeden Could you suggest what is the best entry-point to this? I've read docs/dev/README.md etc but struggling with this. Initially I though it would be enough to just force func cmdCheckUpdate to bail out immediately, but seems the code base has a lot of assumptions regarding forces binary upgrades.

[dveeden]

I'll have a look at this tomorrow as I'm attending a conference today

[xhebox]

Maybe it will be easy to change mirror to local path(yes, we can set it to local filesystem) automatically. Then adding a fallback for unexisted localpath.

[ottok]

What do you mean by local mirror? Which function/code path did you have in mind?

I am trying to avoid using any mirror or copying binaries into users home directories. Instead the existing system binaries should be used as-is.

[xhebox]

What do you mean by local mirror? Which function/code path did you have in mind? I am trying to avoid using any mirror or copying binaries into users home directories. Instead the existing system binaries should be used as-is.

It is possible to set tiup mirror set /home/xxxx, as long as /home/xxx is a valid mirror directory(containing manifests or so). In such case, tiup will not access internet.

If you hack through this path, e.g. IsPackagedBuild will execute tiup mirror set /unexisted_virtual_path automatically. And V1Repo also handle this /unexisted_virtual_path, it will be more consistent. And it should not need to call IsPackageBuild everywhere. IMO, better than adding another layer on top of V1Repo.

BTW: V2Repo is under development currently. I hope eventually these parts can be simplified.

[ottok]

I'll have a look at this tomorrow as I'm attending a conference today

Could you please comment / provide guidance so it is easier for me to finish this with the optimal approach?

[dveeden]

I'll have a look at this tomorrow as I'm attending a conference today

Could you please comment / provide guidance so it is easier for me to finish this with the optimal approach?

I think the local mirror that @xhebox suggested would probably be a good approach.

Disabling the version update check would be good.

However if we disable access to online repository info and components then the functionality of the tool is very limited.

Should we consider shipping a functional local mirror with the package as data?

[dveeden]

dvaneeden@dve-carbon:~/dev/pingcap/tiup$ dlv debug ./ -- list
Type 'help' for list of commands.
(dlv) b (*V1Repository).fetchTimestamp
Breakpoint 1 set at 0xae46b6 for github.com/pingcap/tiup/pkg/repository.(*V1Repository).fetchTimestamp() ./pkg/repository/v1_repository.go:533
(dlv) c
Online version check and repository interaction skipped in packaged build.
> [Breakpoint 1] github.com/pingcap/tiup/pkg/repository.(*V1Repository).fetchTimestamp() ./pkg/repository/v1_repository.go:533 (hits goroutine(1):1 total:1) (PC: 0xae46b6)
   528:	}
   529:	
   530:	// FetchTimestamp downloads the timestamp file, validates it, and checks if the snapshot hash in it
   531:	// has the same value of our local one. (not hashing the snapshot file itself)
   532:	// Return weather the manifest is changed compared to the one in local ts and the FileHash of snapshot.
=> 533:	func (r *V1Repository) fetchTimestamp() (changed bool, manifest *v1manifest.Manifest, err error) {
   534:		// check cache first
   535:		if r.timestamp != nil {
   536:			return false, r.timestamp, nil
   537:		}
   538:	
(dlv) p r
*github.com/pingcap/tiup/pkg/repository.V1Repository nil

So the issue is that r.timestamp is accessed while r is nil.

[dveeden]

Maybe something like this?

diff --git a/cmd/list.go b/cmd/list.go
index e1b889a1..c1cbe97e 100644
--- a/cmd/list.go
+++ b/cmd/list.go
@@ -111,7 +111,13 @@ func showComponentList(env *environment.Environment, opt listOptions) (*listResu
        }
 
        index := v1manifest.Index{}
-       _, exists, err := env.V1Repository().Local().LoadManifest(&index)
+       repo := env.V1Repository()
+
+       if repo == nil {
+               return nil, errors.New("invalid or corrupt repository")
+       }
+
+       _, exists, err := repo.Local().LoadManifest(&index)
        if err != nil {
                return nil, err
        }
diff --git a/pkg/repository/v1_repository.go b/pkg/repository/v1_repository.go
index ca443d90..fc813af3 100644
--- a/pkg/repository/v1_repository.go
+++ b/pkg/repository/v1_repository.go
@@ -531,6 +531,10 @@ func (r *V1Repository) PurgeTimestamp() {
 // has the same value of our local one. (not hashing the snapshot file itself)
 // Return weather the manifest is changed compared to the one in local ts and the FileHash of snapshot.
 func (r *V1Repository) fetchTimestamp() (changed bool, manifest *v1manifest.Manifest, err error) {
+       if r == nil {
+               return false, nil, errors.New("repo is nil")
+       }
+
        // check cache first
        if r.timestamp != nil {
                return false, r.timestamp, nil

[ottok]

Isn't the mirror used to fetch the components that get installed in ~/components/ which include grafana, pd, playground, prometheus, tidb, tiflash and tikv. If we completely disable the mirror or use a local mirror that is empty, then users can't get any of these components, right?

That seems overkill. I just want to bypass the mandatory version check and download for the binaries that are already in the package. Running tiup list or tiup client should not have a mandatory mirror check. Those commands should just run and skip that stage.

I am currently testing Daniël's patch.

[dveeden]

@ottok Yes, I agree with that.

[ottok]

@dveeden Any comments about the overall direction of the latest version of this PR?

[ottok]

There are a few places in the provided code where a nil pointer dereference (segfault) could occur when the IsPackagedBuild flag is true. When IsPackagedBuild is true, the v1Repo field in the Environment struct is explicitly set to nil. Any method call on this nil interface would then cause a panic.

I have an improved version of this commit on my Debian packaging branch, and I will update it here once I get the Debian packaging stabilized.