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

[feat]Curveadm: add debug mode #303

Merged
merged 1 commit into from
Oct 8, 2023
Merged

Conversation

Songjf-ttk
Copy link
Contributor

@Songjf-ttk Songjf-ttk commented Aug 28, 2023

Related to #252.

Solution: Add a debug option to the configuration file curvedm.cfg.

Test:
image
The container was not automatically deleted when an error occurred

@Wine93 Wine93 added this to the v0.3.0 milestone Aug 28, 2023
@Wine93
Copy link
Collaborator

Wine93 commented Aug 28, 2023

Hi @Songjf-ttk, do you think is it more convenient to add a debug flag for format and deploy command?

e.g.

curveadm format --debug 
curveadm deploy --debug

@Songjf-ttk
Copy link
Contributor Author

Songjf-ttk commented Aug 29, 2023

Yes, I think so. I will modify my code as soon as possible. @Wine93

@@ -111,6 +111,7 @@ type deployOptions struct {
insecure bool
poolset string
poolsetDiskType string
debugDeploy bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe debug is more clearly.

filename string
showStatus bool
stopFormat bool
debugFormat bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto.

@@ -238,6 +241,7 @@ func genDeployPlaybook(curveadm *cli.CurveAdm,
} else if step == CREATE_LOGICAL_POOL {
options[comm.KEY_CREATE_POOL_TYPE] = comm.POOL_TYPE_LOGICAL
}
options[comm.DEBUG_MODE] = debugDeploy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sugesstion:

options := map[string]interface{}{
    comm.DEBUG_MODE: debug,
}
if step == CREATE_PHYSICAL_POOL {
    // do something
}
...

showStatus := options.showStatus
stopFormat := options.stopFormat
debugFormat := options.debugFormat
if showStatus && stopFormat {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you can do this in PreRunE phase, see:

PreRunE: func(cmd *cobra.Command, args []string) error {

@Songjf-ttk Songjf-ttk changed the title [feat]CurveadmConfig: add debug mode [WIP][feat]CurveadmConfig: add debug mode Sep 1, 2023
Comment on lines 252 to 255
debugmode := curveadm.MemStorage().Get(comm.DEBUG_MODE).(bool)
if debugmode {
glg.Get().SetLevel(glg.DEBG)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to adjust the log level here. the log level should be determined by the configuration file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two ways to implement debug mode. The first is to directly modify curveadm.cfg and read the log level through the configuration file. The second method is to configure the debug mode through the command line, such as :

curveadm format --debug 
curveadm deploy --debug

After communicating with @Wine93 this time, I chose this solution. This Solution set the debug mode in the command line, which may conflict with the log level in curveadm.cfg, so it is necessary to modify the log level.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are two ways to implement debug mode. The first is to directly modify curveadm.cfg and read the log level through the configuration file. The second method is to configure the debug mode through the command line, such as :

curveadm format --debug 
curveadm deploy --debug

After communicating with @Wine93 this time, I chose this solution. This Solution set the debug mode in the command line, which may conflict with the log level in curveadm.cfg, so it is necessary to modify the log level.

yep, maybe you can provide a function for curveadm to set log level.

if debugmode {
glg.Get().SetLevel(glg.DEBG)
return nil, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

@@ -279,6 +280,7 @@ func NewCreateContainerTask(curveadm *cli.CurveAdm, dc *topology.DeployConfig) (
Init: true,
Name: hostname,
Privileged: true,
Remove: !debugmode,
Copy link
Contributor

Choose a reason for hiding this comment

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

here is a problem, the service container exit but don't delete it forever util delete it manually.

@Songjf-ttk Songjf-ttk changed the title [WIP][feat]CurveadmConfig: add debug mode [feat]CurveadmConfig: add debug mode Sep 18, 2023
@@ -133,6 +170,11 @@ func runFormat(curveadm *cli.CurveAdm, options formatOptions) error {
var err error
var fcs []*configure.FormatConfig
diskRecords := curveadm.DiskRecords()
debugmode := options.debug
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate with line 128?
debug := options.debug
debugmode := options.debug

@caoxianfei1
Copy link
Contributor

@Songjf-ttk look at the other comments.

@caoxianfei1
Copy link
Contributor

@Songjf-ttk
image
The format complete but status is formatting when test it. You may need to lookup format_status.go file and modify the logic.
By the way, should we provide a command such curveadm format --clean? In this way we can delete these format containers that exist but exited because we use curveadm format --debug for avoid skipping when reformatting.
image

@Songjf-ttk Songjf-ttk force-pushed the develop branch 5 times, most recently from 5c0b62e to bec09a1 Compare October 5, 2023 09:05
@Songjf-ttk Songjf-ttk changed the title [feat]CurveadmConfig: add debug mode [feat]Curveadm: add debug mode Oct 5, 2023
@Songjf-ttk
Copy link
Contributor Author

@Songjf-ttk image The format complete but status is formatting when test it. You may need to lookup format_status.go file and modify the logic. By the way, should we provide a command such curveadm format --clean? In this way we can delete these format containers that exist but exited because we use curveadm format --debug for avoid skipping when reformatting. image

  1. The reason status is 'formatting' is because curveadm determines the status based on the container's Status. It has now been fixed:
else if len(*s.containerStatus) > 1 && !strings.Contains(*s.containerStatus, "Exited")
  1. add a command clean to delete the container
curveadm format --clean

@caoxianfei1
Copy link
Contributor

@Songjf-ttk please squash to one commit.

showStatus bool
stopFormat bool
debug bool
cleanFormat bool
Copy link
Contributor

Choose a reason for hiding this comment

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

clean instead of cleanFormat is better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

clean instead of cleanFormat is better?

Agree!

@@ -82,6 +122,8 @@ func NewFormatCommand(curveadm *cli.CurveAdm) *cobra.Command {
flags.StringVarP(&options.filename, "formatting", "f", "format.yaml", "Specify the configure file for formatting chunkfile pool")
flags.BoolVar(&options.showStatus, "status", false, "Show formatting status")
flags.BoolVar(&options.stopFormat, "stop", false, "Stop formatting progress")
flags.BoolVar(&options.debug, "debug", false, "Debug formatting progress")
flags.BoolVar(&options.cleanFormat, "clean", false, "Clean the Container")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

showStatus := options.showStatus
stopFormat := options.stopFormat
debug := options.debug
cleanFormat := options.cleanFormat
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Comment on lines 50 to 53
FORMAT_CHUNKFILE_POOL = playbook.FORMAT_CHUNKFILE_POOL
GET_FORMAT_STATUS = playbook.GET_FORMAT_STATUS
STOP_FORMAT = playbook.STOP_FORMAT
CLEAN_FORMAT = playbook.CLEAN_FORMAT
Copy link
Contributor

Choose a reason for hiding this comment

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

delete it if not used.

Signed-off-by: sjf <s1973853034@163.com>
@caoxianfei1
Copy link
Contributor

lgtm.

Copy link
Collaborator

@Wine93 Wine93 left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants