You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "jiangfucheng (via GitHub)" <gi...@apache.org> on 2023/05/21 14:45:17 UTC
[GitHub] [apisix] jiangfucheng opened a new pull request, #9521: fix: start error bug
jiangfucheng opened a new pull request, #9521:
URL: https://github.com/apache/apisix/pull/9521
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes https://github.com/apache/apisix/issues/9171
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1259574132
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
I think we can fix this bug by https://github.com/apache/apisix/pull/9701#discussion_r1259432308
Do the `cleanup` when abnormal, what do you think? @jiangfucheng
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1260029072
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
Then you will have two config file: config.yaml and the custom_config.yaml, how do you decide which one to choose each time you start APISIX?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9521: fix: start error bug
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1259681678
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
I fetch codes of this PR, and test it, it can't resolve this https://github.com/apache/apisix/issues/9171. You can try it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1261483520
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
> which config will be choose depend on --config flag. After APISIX started, we will read .config_name file content to decide which config will be choose.
Looks good, but you still have to copy the custom config file to the conf directory, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1201718431
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
Is this an appropriate change? the customized_yaml won't work if there is a `local_conf_path_bak` file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1259574132
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
I think we can fix this bug by https://github.com/apache/apisix/pull/9701#discussion_r1259432308
Do the `cleanup` when abnormal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a diff in pull request #9521: fix: start error bug
Posted by "tokers (via GitHub)" <gi...@apache.org>.
tokers commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1201802441
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
Indeed I have another option, why do we need a backup file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9521: fix: start error bug
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1261208074
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
> The pr is not right, what i want to say is that _when starting APISIX fails, recover the config.yaml file_. We can do like this:
>
> ```
> if env.deployment_role ~= "data_plane" then
> local err = init_etcd(env, args)
> if err then
> cleanup()
> util.die(err)
> end
> end
> ```
>
> Now we die in the `init_etcd` with no returns, so we should modify the `init_etcd` function to make sure the it returns the error msg instead of just crashing! The key point is `cleanup()`
I'm not sure if only `init_etcd` will cause crash, if so, I think is ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 closed pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 closed pull request #9521: fix: start error bug
URL: https://github.com/apache/apisix/pull/9521
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9521: fix: start error bug
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1201811919
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
Once start APISIX use customized config, the original `config.yaml ` will be `rename` to `config.yaml.bak` and set a hard link from customized config to `config.yaml`, if occur some errors cause start APISIX fail then it will not restore these config files, so we don't need do above opeartion again, just use `config.yaml` as apisix config directly is enough.
Unless the user create `local_conf_path_bak` manually before start apisix.
If we can detected if have errors when start APISIX, it maybe the best solution, but, I have no idea about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1260038026
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
The pr is not right, what i want to say is that *when starting APISIX fails, recover the config.yaml file*.
We can do like this:
```
if env.deployment_role ~= "data_plane" then
local err = init_etcd(env, args)
if err then
cleanup()
util.die(err)
end
end
```
Now we die in the `init_etcd` with no returns, so we should modify the `init_etcd` function to make sure the it returns the error msg instead of just crashing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9521: fix: start error bug
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1262609952
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
Emm, In current logic, you are right. Give me some time to test it, I think it can be solved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9521: fix: start error bug
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1259707605
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
I think @tokers is right, we should find a way to access customized config file directly, but not rename the customized config file to origin `config.yaml`, it brings too many problems.
I have a simple idea, we can save the custom config file name to a sparate file(like `.config_name`) when we start apisix, so we can access it directly when we need it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9521:
URL: https://github.com/apache/apisix/pull/9521#issuecomment-1639947993
I will close this pr because it is fixed by https://github.com/apache/apisix/pull/9846
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9521: fix: start error bug
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1261205245
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
> Then you will have two config file: config.yaml and the custom_config.yaml, how do you decide which one to choose each time you start APISIX?
which config will be choose depend on `--config` flag. After APISIX started, we will read `.config_name` file content to decide which config will be choose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1260038026
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
The pr is not right, what i want to say is that *when starting APISIX fails, recover the config.yaml file*.
We can do like this:
```
if env.deployment_role ~= "data_plane" then
local err = init_etcd(env, args)
if err then
cleanup()
util.die(err)
end
end
```
Now we die in the `init_etcd` with no returns, so we should modify the `init_etcd` function to make sure the it returns the error msg instead of just crashing!
The key point is `cleanup()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1260038026
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
The pr is not right, what i want to say is that *when starting APISIX fails, recover the config.yaml file*.
So we can do like this:
```
if env.deployment_role ~= "data_plane" then
local err = init_etcd(env, args)
if err then
cleanup()
util.die(err)
end
end
```
Now we die in the `init_etcd` with no returns, so we should modify the `init_etcd` function to make sure the it returns the error msg instead of just crashing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9521: fix: start error bug
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9521:
URL: https://github.com/apache/apisix/pull/9521#discussion_r1261483520
##########
apisix/cli/ops.lua:
##########
@@ -807,17 +807,19 @@ local function start(env, ...)
local local_conf_path = profile:yaml_path("config")
local local_conf_path_bak = local_conf_path .. ".bak"
- local ok, err = os_rename(local_conf_path, local_conf_path_bak)
- if not ok then
- util.die("failed to backup config, error: ", err)
- end
- local ok, err1 = lfs.link(customized_yaml, local_conf_path)
- if not ok then
- ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not pl_path.exists(local_conf_path_bak) then
+ local ok, err = os_rename(local_conf_path, local_conf_path_bak)
if not ok then
- util.die("failed to recover original config file, error: ", err)
+ util.die("failed to backup config, error: ", err)
+ end
+ local ok, err1 = lfs.link(customized_yaml, local_conf_path)
+ if not ok then
+ ok, err = os_rename(local_conf_path_bak, local_conf_path)
+ if not ok then
+ util.die("failed to recover original config file, error: ", err)
+ end
+ util.die("failed to link customized config, error: ", err1)
end
- util.die("failed to link customized config, error: ", err1)
end
Review Comment:
> which config will be choose depend on --config flag. After APISIX started, we will read .config_name file content to decide which config will be choose.
Looks good, but you still have to copy the custom config file to the conf directory, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org