Merge master to feature/host-network-device-ordering #6479
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unused. Avoids having to maintain the function when the code around it changes. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
This should fix the current xs-opam failure with xapi master. Fixes: bfea6f3 ("CA-409628: Add backtrace logging test") Signed-off-by: Andrii Sultanov <andriy.sultanov@vates.tech>
This should fix the current xs-opam failure with xapi master, see for example the failure here: https://github.com/last-genius/xs-opam/tree/master Fixes: bfea6f3 ("CA-409628: Add backtrace logging test")
The current default count of metadata backup files is 25. This number is too many. For the customer in XSI-1873, the backup disk is 500 MB, and each backup file is about 35 MB. So it can't keep 25 backup files. A quick solution is to modify the backup parameters, including: 1. Reduce the default count of metadata backup files to 12. 2. Increase the backup VDI size to 1 GiB so it can keep more backup files. Signed-off-by: Bengang Yuan <bengang.yuan@cloud.com>
The current default count of metadata backup files is 25. This number is too many. For the customer in XSI-1873, the backup disk is 500 MB, and each backup file is about 35 MB. So it can't keep 25 backup files. A quick solution is to modify the backup parameters, including: 1. Reduce the default count of metadata backup files to 12. 2. Increase the backup VDI size to 1 GiB so it can keep more backup files.
When the VM (and its memory) isn't actually going to be moved anywhere (like in VDI migration to another SR), there's no point in ballooning down, it's actually likely to make VDI migration take longer if swap is engaged. Instead change the ballooning target to memory_actual and wait for any ballooning to be stopped. If no ballooning could have been happening in the first place (dynamic_min = dynamic_max = static_max), then don't do any ballooning manipulations at all. Signed-off-by: Andrii Sultanov <andriy.sultanov@vates.tech>
When the VM (and its memory) isn't actually going to be moved anywhere (like in VDI migration to another SR), there's no point in ballooning down, it's actually likely to make VDI migration take longer if swap is engaged.
CA-391651 replaced the function `rrd_add_ds` with an unsafe function `rrd_add_ds_unsafe` in `rrdd_monitor.ml`. Although it has checked if the new ds exists in `rrdi.dss`, if a ds exists in `rrdi.rrd` but not in `rrdi.dss`, it leads the ds duplicates twice in `rrdi.rrd` (E.g. when rrdd plugin starts, it loads local rrdd backup file into `rrdi.rrd` but leaves `rrdi.dss` empty). Solution: Filter out `new_enabled_dss` based on `rrdi.rrd` instead of `rrdi.dss`. Signed-off-by: Bengang Yuan <bengang.yuan@cloud.com>
CA-391651 replaced the function `rrd_add_ds` with an unsafe function `rrd_add_ds_unsafe` in `rrdd_monitor.ml`. Although it has checked if the new ds exists in `rrdi.dss`, if a ds exists in `rrdi.rrd` but not in `rrdi.dss`, it leads the ds duplicates twice in `rrdi.rrd` (E.g. when rrdd plugin starts, it loads local rrdd backup file into `rrdi.rrd` but leaves `rrdi.dss` empty). Solution: Filter out `new_enabled_dss` based on `rrdi.rrd` instead of `rrdi.dss`.
As described in [#6451](#6451), a xapi event could prevent update_vm from pulling the latest Xenopsd metadata, overwriting it with stale information. In case of suspend, this would make the snapshot unresumable, raising an assert in xenopsd due to incongruities in memory values. Instead pull the xenopsd metadata right before updating DB.power_state in Xapi_vm_lifecycle.force_state_reset_keep_current_operations, eliminating the window for the race. Signed-off-by: Andrii Sultanov <andriy.sultanov@vates.tech>
As described in [#6451](#6451), a xapi event could prevent update_vm from pulling the latest Xenopsd metadata, overwriting it with stale information. In case of suspend, this would make the snapshot unresumable, raising an assert in xenopsd due to incongruities in memory values. Instead pull the xenopsd metadata right before updating DB.power_state in`Xapi_vm_lifecycle.force_state_reset_keep_current_operations`, eliminating the window for the race. Closes #6451
Better progress indication, CLI tunables, and the ability to dump raw data to a directory,
that can be used by `ministat`.
Example usage:
```
mkdir /tmp/pool
dune exec ./bench_pool_field.exe --profile=release -- -d /tmp/pool --quota 20
...
dune exec ./bench_pool_field.exe --profile=release -- -d /tmp/pool --quota 20
for i in 'Db.Pool.get_all_records' 'Rpc.t -> pool_t' 'pool_t -> Rpc.t'; do ~/git/ministat/ministat -s "/tmp/pool/${OLD}/${i}.dat" "/tmp/pool/${NEW}/$i.dat" -c 99.5; done
```
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Write directly to the buffer, instead of writing to a buffer,
returning a string, and writing to a buffer again.
Escaping shows up in performance profiles, because `trusted_on_first_use` field
in the pool contains a JSON, which contains \" characters that need escaping.
Reduces memory allocations from 324.4308 mnw/run to 196.3633 mnw/run.
Slight change in performance:
```
Db.Pool.get_all_records:
N Min Max Median Avg Stddev
x 384 81858.452 513398.88 87109.044 88314.45 21963.255
+ 389 78006.031 559052.12 83431.429 84780.535 24315.561
Difference at 95.0% confidence
-3533.91 +/- 3267.84
-4.00151% +/- 3.63493%
(Student's t, pooled s = 23176.9)
```
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
The non-standard S-expression serializer wraps all strings in `'`.
The lexer also ignores every non-escaped character after a `'`, until it sees the next
`'` or escape char.
So it should be safe to avoid escaping `"`.
Unescaping is unchanged: any character can be escaped with `\`,
and it returns it unchanged after removing the escape char, so this preserves backwards
compatibility when loading an old database.
Escaping shows up in performance profiles, because `trusted_on_first_use` field
in the pool contains a JSON, which contains \" characters that needed escaping.
With this change it won't anymore.
`ministat` confirms that there is an improvement:
```
sexpr_of_json_string:
N Min Max Median Avg Stddev
x 806 1438.7128 63663.127 1490.3661 1594.5095 2192.0548
+ 850 911.23529 48528.173 967.25054 1037.7674 1632.6855
Difference at 95.0% confidence
-556.742 +/- 185.531
-34.9162% +/- 9.28192%
(Student's t, pooled s = 1925.34)
str_of_sexp_json:
N Min Max Median Avg Stddev
x 792 1622.9135 49591.388 1719.5377 1786.3412 1702.6472
+ 893 605.37329 3354.8035 626.51812 636.34457 107.24734
Difference at 95.0% confidence
-1150 +/- 111.92
-64.3772% +/- 2.26583%
(Student's t, pooled s = 1169.88)
```
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
For backwards compatibility the serialized form looks like 20250319T04:16:24Z,
which had to go through several transformations before it was parsed: 2 sscanf, 2 sprintf,
and then the Ptime parser.
Since we're parsing the format with sscanf anyway, add a fastpath that builds a Ptime.t directly
without going through reformatting the string and reparsing it.
This speeds up API replies that contain dates, like pool which contains 'telemetry_next_collection' as a date.
`ministat` confirms:
```
Date.of_iso8601:
N Min Max Median Avg Stddev
x 786 1703.462 98255.061 1796.5826 2031.7502 3858.1277
+ 905 525.1954 73923.347 558.17972 711.02195 2732.7547
Difference at 95.0% confidence
-1320.73 +/- 315.725
-65.0045% +/- 10.3523%
(Student's t, pooled s = 3303.82)
Db.Pool.get_all_records:
N Min Max Median Avg Stddev
x 390 76966.273 498179.5 82995.374 84536.667 21266.749
+ 401 69709.657 546133 74811.568 76379.246 23782.821
Difference at 95.0% confidence
-8157.42 +/- 3147.12
-9.64957% +/- 3.57009%
(Student's t, pooled s = 22577.4)
Rpc.t -> pool_t :
N Min Max Median Avg Stddev
x 554 16945.375 267477.23 17620.482 18195.16 10648.914
+ 594 11432.375 251226.67 12011.373 12493.367 9824.9986
Difference at 95.0% confidence
-5701.79 +/- 1184.38
-31.3369% +/- 5.53746%
(Student's t, pooled s = 10230.9)
str_of_sexp_json:
N Min Max Median Avg Stddev
x 893 605.37329 3354.8035 626.51812 636.34457 107.24734
+ 911 510.23412 3298.9936 523.73684 532.25916 98.007142
Difference at 95.0% confidence
-104.085 +/- 9.47756
-16.3568% +/- 1.36325%
(Student's t, pooled s = 102.685)
```
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
We called the wrong finally here. In general we mark the backtrace in finally as important (which involves formatting it, this could be optimized separately). However Mutex.unlock doesn't raise (in well-behaved code, unless you double unlock), so we can use Fun.protect instead. Hashtbl.find in xapi_local_session.ml raises every time in the RBAC checks, this change avoids the costly backtrace formatting (which was discarded by a try/with later anyway). Signed-off-by: Edwin Török <edwin.torok@cloud.com>
We can use Hashtbl.mem instead of catching the exception from Hashtbl.find.
`ministat` confirms:
```
local_session_hook :
N Min Max Median Avg Stddev
x 1057 117.49169 15160.913 123.5201 142.19665 464.12468
+ 1166 38.851635 14365.19 41.619012 57.978805 423.21452
Difference at 95.0% confidence
-84.2178 +/- 36.8873
-59.2263% +/- 19.5015%
(Student's t, pooled s = 443.137)
```
Although there are also some unexplained, but reproducible slowdowns in code that isn't touched by this commit at all:
```
str_of_sexp_json :
N Min Max Median Avg Stddev
x 911 510.16149 3075.0192 523.44043 531.48578 97.085991
+ 892 605.73462 2951.0948 632.27053 652.10806 95.96889
Difference at 95.0% confidence
120.622 +/- 8.91245
22.6953% +/- 1.88103%
(Student's t, pooled s = 96.5349)
```
Perhaps this is due to code layout changes?
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
read_record was marshaling the maps all the time, and some of these maps can be quite big.
Cache the marshaled form in the row itself.
This is a trade-off between memory usage and performance
(caching increases global memory usage, but avoids repeated allocations every time get_record is called,
which may decrease memory usage/GC pressure under load).
Eventually we may be able to drop the marshaled form, but we need to change the db_rpc remote protocol for that
(it currently cannot marshal/unmarshal values on its own, because it doesn't have access to the type information).
`ministat` confirms:
```
Db.Pool.get_all_records :
N Min Max Median Avg Stddev
x 400 71008.769 489829.75 75874.584 76957.497 20852.389
+ 871 51638.867 469264.11 54400.946 55511.462 19845.992
Difference at 95.0% confidence
-21446 +/- 2387.53
-27.8674% +/- 2.84131%
(Student's t, pooled s = 20167.8)
sexpr_of_json_string :
N Min Max Median Avg Stddev
x 862 819.74359 44758.053 851.53894 915.0725 1496.4717
+ 1688 970.16146 51923.24 1024.1987 1097.3771 1748.4532
Difference at 95.0% confidence
182.305 +/- 136.827
19.9224% +/- 15.8188%
(Student's t, pooled s = 1667.57)
str_of_sexp_json :
N Min Max Median Avg Stddev
x 1896 605.00587 2932.3931 627.51199 640.73142 89.48524
+ 1812 537.56839 3105.1991 550.25275 558.89761 93.633252
Difference at 95.0% confidence
-81.8338 +/- 5.89411
-12.7719% +/- 0.864486%
(Student's t, pooled s = 91.5357)
```
Slight increase in 'sexpr_of_json_string'.
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
This doesn't have feature flags, they are optimizations. The pool object has gained a JSON field recently, which slowed it down significantly, because the `"` was getting escaped (so the fastpath that checks whether there are any escapable characters or not couldn't be triggerred). The date field is also very slow to handle due to various backward compatibility format transformations. Add a direct parser for the format used in memory (it'd be good to skip all these serializing/deserializing, but that is a bigger change). Handling the database requires several `finally` calls, which perform a lot of backtrace formatting, and some code keeps raising exceptions even on happy paths (e.g. Hashtbl.find). We need to fix those too, but we should also speed up `finally`. Xapi_local_session was also raising exceptions all the time, replace this with Hashtbl.mem
Refix with some review comments addressed. To join a host into a pool with cluster enabled, the host must have one and only one IP configured on the joining cluster network. If not, after the host joinied the pool, GFS2 SR cannot be plugged on the joined host because an IP is required in the cluster network. Pool join in this scenario has been blocked in XenCenter, here we will block it inside xapi. Signed-off-by: Gang Ji <gang.ji@cloud.com>
…ameter Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Used 'dune format-dune-file dune >dune && mv dune.tmp dune' No functional change. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
ocaml-lsp-server's 'Action->Infer interface' was used for this, and then removed internal/unused entries. Having an mli file is useful: * can find dead code more easily if it is clear which functions are internal to a module and which aren't * the impact of refactoring changes is more obvious (did we have to change an mli at all?) * makes it easier to understand what a module does There are some .ml files which only contain types, these are instead renamed to .mli and dune's `modules_without_implementation` feature is used. Executables get an empty .mli (recent versions of dune already do the equivalent of this, but this makes it more obvious). Drop code from .ml files that now show up as unused. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
…ameter (#6459) This would eventually lead to compilation errors with newer versions of GCC, best fix it now by renaming the parameter.
ocaml-lsp-server's 'Action->Infer interface' was used for this, and then removed internal/unused entries. Having an mli file is useful: * can find dead code more easily if it is clear which functions are internal to a module and which aren't * the impact of refactoring changes is more obvious (did we have to change an mli at all?) * makes it easier to understand what a module does There are some .ml files which only contain types, these are instead renamed to .mli and dune's `modules_without_implementation` feature is used. Executables get an empty .mli (recent versions of dune already do the equivalent of this, but this makes it more obvious). Drop code from .ml files that now show up as unused. Eventually we can also add some documentation, this will be done in followup PRs for new code.
On XS9 a previiously available SM type might become unavailable. Make sure we remove it on upgrade. The previius fix in CA-408048 did not work. Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
The current logic in storage_migrate.ml for mirror failure check is specific to tapdisk, hence multiplex it. `pre_deactivate_hook` also has something similar to check for mirror failure, so do something similar there. Signed-off-by: Vincent Liu <shuntian.liu2@cloud.com>
Previously this was deleted in commit 1fe6389 as it was not multiplexed, but looks like we still need to keep it in storage_mux because sm-cli needs to make rpc calls to storage_mux when trying to list all the mirrors to make it work properly, due to the fact that the sr plugins are stored in the address space of the xapi process. There are other invocations in sm-cli such as `Storage_migrate.start` which may have similar problems. But I have left them alone as I don't any reasonable way of calling them from the cli. Signed-off-by: Vincent Liu <shuntian.liu2@cloud.com>
So we can call this from more generic code, without worrying whether locks are held. Skip adding to the stringpool when locks aren't held and we can't obtain an atomic lock either. No functional change Signed-off-by: Edwin Török <edwin.torok@cloud.com>
This needs to be called around the same places that `ensure_utf8_xml` and `UTF8_XML.is_valid` gets called: those are the places where new strings can enter the system. Now that calling Share.merge is thread-safe also call it when writing new fields, not just when creating the row. For now the DB lock is still held when this function is called, but future refactoring might move code around. For maps and sets we only call this when new keys get added (except in create_row, where we need to process all elements) Signed-off-by: Edwin Török <edwin.torok@cloud.com>
* schema access, similar to write_field: we can access it outside of create_row_locked, reducing the time we hold the DB lock. * debug writes * UTF8 validation and StringPool operations We cannot move field reads in read/write pairs because we need to ensure noone else has changed the DB after we've read it. (There are other alternatives if we used lock-free data structures, we could optimistically perform the operation on a snapshot, and check whether the snapshot is still the same when committing, and if not repeat the operation. But for now make just simple changes). Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Stores a Value.t and its marshaled form. A phantom type distinguishes between situations where Value.t is always present, from situations where it may be absent. For now it is always present, but may become absent when we start using this for `Db_interface.field`. Document complexity of functions (most are `O(1)`, except set/map at construction time, and unmarshal if the backwards compatible construction from string is used). In particular operations on strings are `O(1)`, because they don't look at the actual string value, just wrap or unwrap it from a `Value.String` variant. set/map requires serialization in S-expression form, but this is only done once at construction time, and we can retrieve it multiple times in `O(1)`. No functional or performance change Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Will make it easier to introduce UTF8 validation here. No functional change Signed-off-by: Edwin Török <edwin.torok@cloud.com>
field=string for now, but will be changed to Schema.CachedValue.t later. There are separate types for input and output fields, e.g. we may want to perform UTF8 validation on input, which would modify both Value.t and its serialized form, so we can avoid serializing twice. No functional change Signed-off-by: Edwin Török <edwin.torok@cloud.com>
This provides direct access to a Value.t when known, which avoids having to serialize and deserialize a Value.t just to read it. Not yet used, no functional change. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
No functional change, we still use the Compat module Signed-off-by: Edwin Török <edwin.torok@cloud.com>
`ministat` confirms a speedup:
```
Db.Pool.get_all_records :
N Min Max Median Avg Stddev
x 432 54115.256 493532.78 56048.42 57937.384 24117.68
+ 524 22642.778 333257.36 23595.495 24679.258 15206.708
Difference at 95.0% confidence
-33258.1 +/- 2513.99
-57.4036% +/- 2.90374%
(Student's t, pooled s = 19737.2)
>>>> Db.VM.set_NVRAM :
N Min Max Median Avg Stddev
x 132 36794 2355369.4 1095736.6 1107222.1 298247.5
+ 168 49167.417 1485278.1 678231.31 685636.89 161480.92
Difference at 95.0% confidence
-421585 +/- 52835.6
-38.0759% +/- 3.54275%
(Student's t, pooled s = 231767)
```
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
A user can install server certificates to secure the connection between xapi and their API clients. So far we demanded SHA256 certificates. Accept SHA512 in addition. The patch renames the predicate to no longer imply that we only accept SHA256. Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
…me (#6462) Builds on top of #6461 Benchmark results are on individual commits. The most significant one: `ministat` confirms a speedup: ``` Db.Pool.get_all_records : N Min Max Median Avg Stddev x 432 54115.256 493532.78 56048.42 57937.384 24117.68 + 524 22642.778 333257.36 23595.495 24679.258 15206.708 Difference at 95.0% confidence -33258.1 +/- 2513.99 -57.4036% +/- 2.90374% (Student's t, pooled s = 19737.2) >>>> Db.VM.set_NVRAM : N Min Max Median Avg Stddev x 132 36794 2355369.4 1095736.6 1107222.1 298247.5 + 168 49167.417 1485278.1 678231.31 685636.89 161480.92 Difference at 95.0% confidence -421585 +/- 52835.6 -38.0759% +/- 3.54275% (Student's t, pooled s = 231767) ```
On XS9 a previiously available SM type might become unavailable. Make sure we remove it on upgrade. The previius fix in CA-408048 did not work.
Mention the supported algorithms, re-arrange the wording to usa list. Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
A user can install server certificates to secure the connection between xapi and their API clients. So far we demanded SHA256 certificates. Accept SHA512 in addition. The patch renames the predicate to no longer imply that we only accept SHA256. Tested this: ``` openssl req -x509 -sha512 -nodes -days 365 -newkey rsa:2048 -keyout mycert.key -out mycert.crt -subj "/CN=localhost" xe host-server-certificate-install certificate=mycert.crt private-key=mycert.key ```
This is done using the API errors. The use of Hashtbl.find is warranted, if the name is missing from the table something very bad has happened and would have had to raise an error anyway. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Each Parallel atom takes up a worker thread whilst its children do the actual work, so we have parallel_queues to prevent a deadlock. However, nested Parallel atoms take up an additional worker, meaning they can still cause a deadlock. This commit adds a new Nested_parallel atomic with matching nested_parallel_queues to remove the possibility of this deadlock. This increases the total number of workers, but these workers are just to hold the Nested_parallel Atomics and will not be doing any actual work Signed-off-by: Steven Woods <steven.woods@cloud.com>
This is done using the API errors, and quite useful when adding tests or changing them to interpret the results
Back in 2018 a4a94b3 rejected empty IQNs in set_iscsi_iqn API calls. However hosts are created with an empty IQN, and if this code runs too early then it will attempt to call `set-iscsi-initiator` with an empty string for the IQN: ``` /opt/xensource/libexec/set-iscsi-initiator myhost ``` About a second later the script is called again with the correct value. This could potentially result in the iscsid service being restarted multiple times (and if a restart is still pending when restart is called a 2nd time I'm not sure it'll take effect, so we might be left with an empty initiator). It is best to avoid setting empty initiators. The exception would be raised and ignore due to the log_and_ignore in the caller. Also log wherever the IQN is set using %S, so that we notice if it ends up containing some extra whitespace characters. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
This is a stopgap until we add compile-time constraints on the nesting, by for example using a polymorphic variant. Signed-off-by: Steven Woods <steven.woods@cloud.com>
…QN (#6474) Back in 2018 a4a94b3 rejected empty IQNs in set_iscsi_iqn API calls. However hosts are created with an empty IQN, and if this code runs too early then it will attempt to call `set-iscsi-initiator` with an empty string for the IQN: ``` /opt/xensource/libexec/set-iscsi-initiator myhost ``` About a second later the script is called again with the correct value. This could potentially result in the iscsid service being restarted multiple times (and if a restart is still pending when restart is called a 2nd time I'm not sure it'll take effect, so we might be left with an empty initiator, I have also seen a GFS2 SR plug failure following this). It is best to avoid setting empty initiators. The exception would be raised and ignore due to the log_and_ignore in the caller. Also log wherever the IQN is set using %S, so that we notice if it ends up containing some extra whitespace characters.
Each Parallel atom takes up a worker thread whilst its children do the actual work, so we have parallel_queues to prevent a deadlock. However, nested Parallel atoms take up an additional worker, meaning they can still cause a deadlock. This commit adds a new Nested_parallel atomic with matching nested_parallel_queues to remove the possibility of this deadlock. This increases the total number of workers, but these workers are just to hold the Nested_parallel Atomics and will not be doing any actual work
Helpers.progress' only user was Xapi_vm.immediate_complete, which wasn't used anywhere. Remove both. No functional changes. Signed-off-by: Andrii Sultanov <andriy.sultanov@vates.tech>
The paths to SM plugins in XS9 is about to change. This will be implemented with a conf file in /etc/xapi.conf.d/ and will be easy to overlook when just looking at xapi.conf. Update the comment there. Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
`Helpers.progress`' only user was `Xapi_vm.immediate_complete`, which wasn't used anywhere. Remove both. No functional changes.
The paths to SM plugins in XS9 is about to change. This will be implemented with a conf file in /etc/xapi.conf.d/ and will be easy to overlook when just looking at xapi.conf. Update the comment there.
BengangY
approved these changes
May 21, 2025
gangj
approved these changes
May 21, 2025
changlei-li
merged commit 9c78090
into
feature/host-network-device-ordering
64 of 65 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.