Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor mc cp and make session optional #2910

Merged
merged 1 commit into from
Oct 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions cmd/cp-main.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ var (
Name: "attr",
Usage: "add custom metadata for the object",
},
cli.BoolFlag{
Name: "continue, c",
Usage: "create or resume copy session.",
},
}
)

Expand Down Expand Up @@ -127,6 +131,9 @@ EXAMPLES:

13. Copy a text file to an object storage and assign REDUCED_REDUNDANCY storage-class to the uploaded object.
{{.Prompt}} {{.HelpName}} --storage-class REDUCED_REDUNDANCY myobject.txt play/mybucket

14. Copy a text file to an object storage and create or resume copy session.
$ {{.HelpName}} --recursive --continue myobject.txt play/mybucket
`,
}

Expand Down Expand Up @@ -431,7 +438,7 @@ loop:
// For critical errors we should exit. Session
// can be resumed after the user figures out
// the problem.
session.CloseAndDie()
session.copyCloseAndDie(session.Header.CommandBoolFlags["session"])
}
}
}
Expand Down Expand Up @@ -497,14 +504,21 @@ func mainCopy(ctx *cli.Context) error {
}
sse := ctx.String("encrypt")

session := newSessionV8()
sessionID := getHash("cp", ctx.Args())
if ctx.Bool("continue") && isSessionExists(sessionID) {
resumeSession(sessionID)
return nil
}

session := newSessionV8(sessionID)
session.Header.CommandType = "cp"
session.Header.CommandBoolFlags["recursive"] = recursive
session.Header.CommandStringFlags["older-than"] = olderThan
session.Header.CommandStringFlags["newer-than"] = newerThan
session.Header.CommandStringFlags["storage-class"] = storageClass
session.Header.CommandStringFlags["encrypt-key"] = sseKeys
session.Header.CommandStringFlags["encrypt"] = sse
session.Header.CommandBoolFlags["session"] = ctx.Bool("continue")
session.Header.UserMetaData = userMetaMap

var e error
Expand Down
16 changes: 14 additions & 2 deletions cmd/session-v8.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func loadSessionV8(sid string) (*sessionV8, *probe.Error) {
}

// newSessionV8 provides a new session.
func newSessionV8() *sessionV8 {
func newSessionV8(sessionID string) *sessionV8 {
s := &sessionV8{}
s.Header = &sessionV8Header{}
s.Header.Version = globalSessionConfigVersion
Expand All @@ -177,7 +177,7 @@ func newSessionV8() *sessionV8 {
s.Header.UserMetaData = make(map[string]string)
s.Header.When = UTCNow()
s.mutex = new(sync.Mutex)
s.SessionID = newRandomID(8)
s.SessionID = sessionID

sessionDataFile, err := getSessionDataFile(s.SessionID)
fatalIf(err.Trace(s.SessionID), "Unable to create session data file \""+sessionDataFile+"\".")
Expand Down Expand Up @@ -375,6 +375,18 @@ func (s sessionV8) CloseAndDie() {
console.Fatalln("Session safely terminated. To resume session `mc session resume " + s.SessionID + "`")
}

func (s sessionV8) copyCloseAndDie(sessionFlag bool) {
if sessionFlag {
s.Close()
console.Fatalln("Command terminated safely. Run this command to resume copy again.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: This would result in exit code being non-zero. Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CloseAndDie() does the same thing. I am keeping the same behaviour.

} else {
s.mutex.Lock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why do we need to do anything at all when session flag wasn't passed along with mc cp ...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. I am just keeping session behaviour same as CloseAndDie(). When mc session command is removed, type sessionV8 may clean up.

defer s.mutex.Unlock()

s.DataFP.Close() // ignore error.
}
}

// Create a factory function to simplify checking if
// object was last operated on.
func isLastFactory(lastURL string) func(string) bool {
Expand Down
13 changes: 13 additions & 0 deletions cmd/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package cmd

import (
"crypto/sha256"
"encoding/hex"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -136,3 +138,14 @@ func removeSessionDataFile(sid string) {
}
os.Remove(dataFile)
}

func getHash(prefix string, args []string) string {
hasher := sha256.New()
for _, arg := range args {
if _, err := hasher.Write([]byte(arg)); err != nil {
panic(err)
}
}

return prefix + "-" + hex.EncodeToString(hasher.Sum(nil))
}
4 changes: 2 additions & 2 deletions cmd/session_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ func (s *TestSuite) TestSession(c *C) {
c.Assert(err, IsNil)
c.Assert(isSessionDirExists(), Equals, true)

session := newSessionV8()
session := newSessionV8(getHash("cp", []string{"mybucket", "myminio/mybucket"}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit

Suggested change
session := newSessionV8(getHash("cp", []string{"mybucket", "myminio/mybucket"}))
session := newSessionV8(getHash("cp", []string{"object", "myminio/mybucket"}))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. I am simulating mc cp -r -c mybucket myminio/mybucket here.

c.Assert(session.Header.CommandArgs, IsNil)
c.Assert(len(session.SessionID), Equals, 8)
c.Assert(len(session.SessionID) >= 8, Equals, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about the following assert? It asserts a more stronger property than >= 8. It uses the fact that the sha256 checksum part of the sessionID is 32 bytes.

Suggested change
c.Assert(len(session.SessionID) >= 8, Equals, true)
c.Assert(len(session.SessionID) >= sha256.Size, Equals, true)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. Existing session support generates 8 bytes long random string whereas this PR generates guessable session ID using SHA-256 where the length is 32 bytes long. This change makes both to work.

When session support is deprecated and removed, this check will be fine-tuned.

_, e := os.Stat(session.DataFP.Name())
c.Assert(e, IsNil)

Expand Down